Fix DELETE /api/oauth/client/🆔 client.remove is not a function
OAuthClient wraps @simpleworkjs/orm's Resource model, whose instance delete method is .delete() -- not .remove(), which is what model-redis's Table instances (e.g. this app's ApiToken, AuthToken) use. The DELETE route called the wrong one, so every delete silently 500'd; the route's try/catch turned it into a plain JSON error response rather than a thrown exception, and the existing tests' cleanup-only delete calls (afterAll, end of the rotate test) never checked the response status, so the bug shipped unnoticed. The Directory Management UI was never affected -- routes/api_directory_admin.js's DELETE routes already used .delete() correctly throughout. Found and root-caused live against a real deployment's SSO API, then reproduced and fixed against a local docker stack with a rebuilt image: confirmed DELETE returned a genuine 500 before the fix and a real 200 + 404-on-subsequent-GET after. Adds two dedicated tests (PUT and DELETE persistence, each verified by a follow-up GET rather than trusting the mutating response alone), and hardens the existing rotate test's incidental delete call with real assertions. Verified the new DELETE test fails on the old code and passes on the fix. Full suite (189 tests, real LDAP + Redis) passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -97,7 +97,7 @@ router.delete('/:client_id', async function(req, res, next) {
|
||||
await permission.byGroup(req.user, [ADMIN_GROUP]);
|
||||
|
||||
const client = await OAuthClient.get(req.params.client_id);
|
||||
await client.remove();
|
||||
await client.delete();
|
||||
|
||||
return res.json({
|
||||
client_id: req.params.client_id,
|
||||
|
||||
@@ -69,6 +69,51 @@ describe('OAuth client management API — /api/oauth/client', () => {
|
||||
expect(res.body.results).not.toHaveProperty('client_secret_hash');
|
||||
});
|
||||
|
||||
test('PUT persists — a changed name survives a fresh GET', async () => {
|
||||
const created = await request(app)
|
||||
.post('/api/oauth/client/')
|
||||
.set('auth-token', token)
|
||||
.send({ name: 'put-persist-test', redirect_uris: REDIRECT_URI });
|
||||
expect(created.status).toBe(200);
|
||||
const id = created.body.results.client_id;
|
||||
|
||||
const updated = await request(app)
|
||||
.put(`/api/oauth/client/${id}`)
|
||||
.set('auth-token', token)
|
||||
.send({ name: 'put-persist-test-renamed' });
|
||||
expect(updated.status).toBe(200);
|
||||
expect(updated.body.results.name).toBe('put-persist-test-renamed');
|
||||
|
||||
const fetched = await request(app).get(`/api/oauth/client/${id}`).set('auth-token', token);
|
||||
expect(fetched.status).toBe(200);
|
||||
expect(fetched.body.results.name).toBe('put-persist-test-renamed');
|
||||
|
||||
await request(app).delete(`/api/oauth/client/${id}`).set('auth-token', token);
|
||||
});
|
||||
|
||||
// Regression: this route called client.remove(), but OAuthClient wraps
|
||||
// @simpleworkjs/orm's Resource model, whose instance method is .delete()
|
||||
// — .remove() doesn't exist on it (unlike the model-redis Tables
|
||||
// elsewhere in this app, e.g. api_token.js, which really do have
|
||||
// .remove()). The route's try/catch turned the resulting TypeError into
|
||||
// a plain 500 JSON response rather than a thrown exception, so every
|
||||
// prior DELETE call in this file's cleanup hooks silently "succeeded"
|
||||
// from Jest's point of view while leaving the client un-deleted.
|
||||
test('DELETE persists — the client is actually gone, not just a 200', async () => {
|
||||
const created = await request(app)
|
||||
.post('/api/oauth/client/')
|
||||
.set('auth-token', token)
|
||||
.send({ name: 'delete-persist-test', redirect_uris: REDIRECT_URI });
|
||||
expect(created.status).toBe(200);
|
||||
const id = created.body.results.client_id;
|
||||
|
||||
const deleted = await request(app).delete(`/api/oauth/client/${id}`).set('auth-token', token);
|
||||
expect(deleted.status).toBe(200);
|
||||
|
||||
const fetched = await request(app).get(`/api/oauth/client/${id}`).set('auth-token', token);
|
||||
expect(fetched.status).toBe(404);
|
||||
});
|
||||
|
||||
test('list then rotate a client by its returned client_id (the bootstrap path)', async () => {
|
||||
// Reproduces exactly what the theta-env bootstrap does: create, list,
|
||||
// find by name, rotate by the client_id from the list response. Uses a
|
||||
@@ -90,7 +135,11 @@ describe('OAuth client management API — /api/oauth/client', () => {
|
||||
expect(rotated.status).toBe(200);
|
||||
expect(rotated.body.client_secret).toBeTruthy();
|
||||
|
||||
await request(app).delete(`/api/oauth/client/${found.client_id}`).set('auth-token', token);
|
||||
const deleted = await request(app).delete(`/api/oauth/client/${found.client_id}`).set('auth-token', token);
|
||||
expect(deleted.status).toBe(200);
|
||||
|
||||
const afterDelete = await request(app).get(`/api/oauth/client/${found.client_id}`).set('auth-token', token);
|
||||
expect(afterDelete.status).toBe(404);
|
||||
});
|
||||
|
||||
test('GET /:id unknown id returns 404, not 500', async () => {
|
||||
|
||||
Reference in New Issue
Block a user