From 7f2c71299fb0ef023b73070acf22ea0eef718464 Mon Sep 17 00:00:00 2001 From: William Mantly Date: Mon, 27 Jul 2026 21:03:34 -0400 Subject: [PATCH] Fix DELETE /api/oauth/client/:id: 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 --- nodejs/routes/oauth_client.js | 2 +- nodejs/tests/oauth.test.js | 51 ++++++++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/nodejs/routes/oauth_client.js b/nodejs/routes/oauth_client.js index 18569cc..b35213f 100644 --- a/nodejs/routes/oauth_client.js +++ b/nodejs/routes/oauth_client.js @@ -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, diff --git a/nodejs/tests/oauth.test.js b/nodejs/tests/oauth.test.js index 5613a1f..ffd0764 100644 --- a/nodejs/tests/oauth.test.js +++ b/nodejs/tests/oauth.test.js @@ -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 () => {