API for enabling/disabling a user #1775

jordigh · 2025-08-18T22:54:07Z

This adds two new endpoints for enabling or disabling a user.

POST /api/users/:userId/disable disables a user
POST /api/users/:userId/enable re-enables a user

A disabled user should not be able to log in. If they attempt, they will receive a 403 HTTP forbidden error code.

Attempts to use the API token of a disabled user will also result in 403.

Frontend details (an interface for managing users, displaying information about disabled users) are not in scope here.

A disabled user that is re-enabled is able to come back as if nothing had happened.

Original commit messages:

1d4e2cc User: add a disabledAt property

This is in analogy with removedAt for documents. If the property is
not null, then the user is considered disabled. To re-enable a user,
we null the property again.
cc5e614 Authorizer: disallow disabled users from logging in

A clear 403 is what they get if they try to log in again.
c96ba2c HomeDBManager: allow changing the disabledAt property of a user

Simply saving the new property to the db upon a change.
2fe3648 API: add /api/users/:userId/[enable|disable] endpoints

These API endpoints allows disabling or enabling a user.
718fc95 HomeDBManaager: remove doc access for disallowed users

If there is an existing websocket connexion, disallowed users might
still be able to use it to send data. There is a 5 second cache on the
doc implementation, but permissions are rechecked after the cache
expires. Checking if a user is disabled here when getting the document
should prevent the user from doing any further work on a document.
8fe1476 ApiServer: add tests for new user disable/enable endpoints

Testing the API endpoints work and also a simple test that a banned
user can't reach an endpoint via API.

dsagal

I know it's a draft, and I only skimmed at high level. Just wanted to nudge towards referring to this as "disabling users", which is a rare state (like banning or blocking), and for this reason I think it would be clearer if new fields were named "disabled" -- or better (by analogy with removedAt on documents and workspaces), "disabledAt", with a timestamp, rather than "isEnabled". And for method names and API calls, it would also be nice to make it rhyme with other things: like there is a DELETE /api/docs/:docId and soft-delete POST /api/docs/:docId/remove; similarly disabling could be the "soft-delete" analog to deleting a user, with a similarly-rhyming endpoint, and similar name (disableUser, like the "soft" version of deleteUser).

Also I noticed you iterate through connections in DocClients, but that's not all the clients, you'd be missing those who have connected (and authenticated) but haven't opened a document. That's why I suggested using Comm.ts to find all websocket connections for a user.

jordigh · 2025-08-19T01:00:26Z

I like to reduce the amount of negative properties. It's like keeping track of minus signs in a calculation. "if not is not enabled" adds a bit of cognitive load, at least for me.

This is not about the negative of "enabled", this is about soft-deleting. I find it more cognitively weird to think about an "enabled" user, it's like thinking about an "enabled" file to represent a file that hasn't been moved to trash.

paulfitz · 2025-08-20T14:01:16Z

I like to reduce the amount of negative properties. It's like keeping track of minus signs in a calculation. "if not is not enabled" adds a bit of cognitive load, at least for me.

This is not about the negative of "enabled", this is about soft-deleting. I find it more cognitively weird to think about an "enabled" user, it's like thinking about an "enabled" file to represent a file that hasn't been moved to trash.

What about a removed_at column, consistent with soft-deleted docs and workspaces? Then we'd also have a timestamp.

If the choice remains enabled versus disabled, I'd prefer disabled because it is an uncommon state.

jordigh · 2025-08-20T14:05:29Z

Should removing be undoable? Maybe I'm stuck thinking that removing/unremoving should both be supported. But maybe once a user is removed, there's no need to reinstate them.

paulfitz · 2025-08-21T13:40:43Z

Should removing be undoable? Maybe I'm stuck thinking that removing/unremoving should both be supported. But maybe once a user is removed, there's no need to reinstate them.

for docs and workspaces, soft-deletion is undoable (removed_at gets reset to null)

This is in analogy with removedAt for documents. If the property is not null, then the user is considered disabled. To re-enable a user, we null the property again.

Simply saving the new property to the db upon a change.

These API endpoints allows disabling or enabling a user.

If there is an existing websocket connexion, disallowed users might still be able to use it to send data. There is a 5 second cache on the doc implementation, but permissions are rechecked after the cache expires. Checking if a user is disabled here when getting the document should prevent the user from doing any further work on a document.

Testing the API endpoints work and also a simple test that a banned user can't reach an endpoint via API.

fflorent

Few remarks so far, but I haven't reviewed everything yet. The code is pretty clear, sounds promising!

test/server/lib/Authorizer.ts

app/gen-server/ApiServer.ts

fflorent

The code is pretty clean, thank you very much @jordigh!

My main worry is about the effectiveness of the user deactivation, I see in the tests that you explicitly call flushDocAuthCache(). Would that mean that an attacker could continue working on a document through WebSocket as long as the DocAuth cache exists?

(I hope my question is clear enough, if not, sorry for that, I will double the effort for expressing my point)

app/gen-server/ApiServer.ts

test/gen-server/ApiServer.ts

test/server/lib/GranularAccess.ts

fflorent

Thank you @jordigh, that will definitely be a very good addition!

vviers · 2025-09-04T12:37:58Z

Looks good to me too and absolutely fits my need! 👌🏼
Thanks @jordigh!

paulfitz

Looking pretty good, thanks @jordigh, some comments.

paulfitz · 2025-09-10T18:48:13Z

app/gen-server/lib/homedb/HomeDBManager.ts

      return doc;
    }
+
+    const fullUser = await this._usersManager.getFullUser(userId);


This is an extra database call in a fairly busy spot. And the only thing it needs to get is user.disableAt? For any regular API call, the user wouldn't get anywhere near this code without the user record having been already read by middleware. For the websocket connection, likewise an authorization check would have happened, which would include reading the user record. It does get cached but as you note elsewhere the cache doesn't last long. I'll come back to this when I've read rest of PR to see if there's anything better we can do that you're not already doing.

Okay back. In the "normal" path through this method, there is a query constructed by _doc, with a few extra things we need added on with methods like _addIsSupportWorkspace and _addFeatures. You could pull out whether the user has been deleted into the query result. It is a bit weird since it is unrelated to the document but it would work and save a round trip? There are other paths through this code, it is ok to relax for the lesser traveled paths and make more database round trips.

Another option is to intervene in the part of the code that computes the queries needed to determine whether a user has access to a resource, and make it aware that users can be disabled.

I've tried your suggestion to add a join to the query, and it seems to work.

paulfitz · 2025-09-10T18:49:55Z

app/gen-server/migration/1754077317821-UserDisabledAt.ts

+
+      await queryRunner.createIndex("users", new TableIndex({
+        name: "users__disabled_at",
+        columnNames: ["disabled_at"],


What is the use case in mind in adding an index for the disabled_at column?

Silly copy-paste without much thought. We never search by disabled_at right now, so there's no need for an index. I've removed it.

test/gen-server/ApiServer.ts

paulfitz · 2025-09-10T18:59:52Z

test/gen-server/ApiServer.ts

      ref: chimpyRef,
      name: "Chimpy",
      picture: null,
+      disabledAt: null,


In other new optional and nullable fields we've added, we've chosen not to sent them when null. This makes for less noise in diffs.

grist-core/app/server/lib/requestUtils.ts

Lines 240 to 249 in ad636a4

// Do not include removedAt field if it is not set. It is not relevant to regular

// situations where the user is working with non-deleted resources.

if (key === 'removedAt' && value === null) { return undefined; }

// Don't bother sending option fields if there are no options set.

if (key === 'options' && value === null) { return undefined; }

// Don't prune anything that is explicitly allowed.

if (allowedFields?.has(key)) { return value; }

// User connect id is not used in regular configuration, so we remove it from the response, when

// it's not filled.

if (key === 'connectId' && value === null) { return undefined; }

Oh, good idea, I've done that.

dsagal · 2025-09-22T15:33:02Z

app/common/UserAPI.ts

  orgAccess?: roles.BasicRole|null;
  anonymous?: boolean;    // If set to true, the user is the anonymous user.
  isMember?: boolean;
+  disabledAt?: Date|null;


A brief comment would be nice (also in app/common/LoginSessionAPI.ts)

Okay, sure.

dsagal · 2025-09-22T16:05:17Z

app/gen-server/ApiServer.ts

+  private async _changeUserDisabledDate(req: express.Request, disabledAt: Date | null) {
+    const isAuthorized = await this._gristServer.getInstallAdmin().isAdminReq(req);
+    if (!isAuthorized) {
+      throw new ApiError('Only admin users can disable or re-enable users', 401);


I think 403 would the right code here ("not allowed" rather than "not authenticated")

BTW, what do you think of using middleware instead, like:

const requireInstallAdmin = this._gristServer.getInstallAdmin().getMiddlewareRequireAdmin(); this._app.post('/api/users....', requireInstallAdmin, expressWrap(...))

(That's how I see admin-protected endpoints checked in FlexServer and attachEarlyEndpoints.ts)

Sure. I was doing it by analogy with other 401s in this file, but I guess they should all be 403s too. Would you like that change too for the other 401s, such as /api/profile/isConsultant or api/profile/allowGoogleIn? Those return 401, which come to think of it, is probably also wrong.

The middleware works, but it will give a less specific error message. I guess that's fine?

app/gen-server/lib/homedb/HomeDBManager.ts

dsagal · 2025-09-22T17:16:46Z

app/server/lib/Authorizer.ts


-  // If no userId has been found yet, fall back on anonymous.
+  if (mreq.user?.disabledAt) {
+    return res.status(403).send('Forbidden');


Wouldn't this prevent access to public URLs? Have you considered instead treating a disabled user as anonymous? I can imagine arguments for this behavior, but then it would be nice to have those spelled out in a comment somewhere.

Yes, all public URLs would be banned too, including public templates or forms.

I've made a small modification here to allow the banned user the opportunity to know that they're logged in as a banned user and they can log out if desired and see public URLs too.

I've also added a comment as you suggest to explain this choice.

Hmm, ok, but this screenshot is making me worried: it's showing the admin user being disabled... Have you tested this in a server with multiple users?

Yes, I have tested it with multiple users.

app/server/lib/requestUtils.ts

app/gen-server/ApiServer.ts

test/gen-server/lib/emails.ts

app/gen-server/lib/homedb/HomeDBManager.ts

dsagal

Some more thoughts:

Could there be a test of behavior for a disabled user who tries to make a call with an API key or with a session cookie -- both to a document, and outside of a document? The tests already here presumably test one of these, but it's hard to tell which, and probably don't test both.
I'd be interested a test case of starting a websocket connection, disabling that user, then making an openDoc call. I am worried that the userId is permanently associated with a Client object -- once there, there is only one method that ever checks for it being disabled. Might it be a good idea to destroy Client objects for a user who got disabled? (I think that was part of an earlier version perhaps)

Also, what happens when a disabled user tries to sign in? I don't see a test case for that. If a test is hard to add, have you tested this manually?

app/gen-server/ApiServer.ts

dsagal

Thank you! BTW, a reminder about another comment I made in #1775 (review), in case it got lost.

dsagal · 2025-09-24T23:00:50Z

app/server/lib/Authorizer.ts

+    // Otherwise the 403 error page on the client side can't get an
+    // active user and thinks the user isn't logged in at all, which
+    // can be more confusing than necessary.
+    && !(['/session/access/active', '/session/access/all'].includes(mreq.url))


Is this intentionally allowing all methods, or should it only allow GET? All might make sense, since this may allow a disabled user to switch to another account if they are signed into multiple. Though it's still the same person whom we presumably don't want, so maybe no need to make switching convenient? Anyway, whatever your decision, could you add a comment about allowed methods?

The intent was to only allow GET. I've made that intent explicit now.

dsagal · 2025-09-24T23:10:16Z

app/server/lib/Authorizer.ts


-  // If no userId has been found yet, fall back on anonymous.
+  if (mreq.user?.disabledAt) {
+    return res.status(403).send('Forbidden');


Hmm, ok, but this screenshot is making me worried: it's showing the admin user being disabled... Have you tested this in a server with multiple users?

jordigh

I've added a couple of tests that explicitly test for an API key and for openDoc. I think deleting existing Client instances is ultimately unnecessary because websocket connexions regularly re-check their permissions for each command sent, whenever the 5-second cache expires.

As for logging in, Grist itself doesn't control whether users can log in or not (authentication). All I can handle on Grist's side is what to do with users once they've logged in (authorization). The solution here is that a disabled, logged-in user gets permission denied for almost everything.

jordigh · 2025-09-25T17:22:35Z

app/server/lib/Authorizer.ts

+    // Otherwise the 403 error page on the client side can't get an
+    // active user and thinks the user isn't logged in at all, which
+    // can be more confusing than necessary.
+    && !(['/session/access/active', '/session/access/all'].includes(mreq.url))


The intent was to only allow GET. I've made that intent explicit now.

jordigh · 2025-09-25T17:29:54Z

app/server/lib/Authorizer.ts


-  // If no userId has been found yet, fall back on anonymous.
+  if (mreq.user?.disabledAt) {
+    return res.status(403).send('Forbidden');


Yes, I have tested it with multiple users.

dsagal · 2025-09-25T20:09:54Z

I've added a couple of tests that explicitly test for an API key and for openDoc. I think deleting existing Client instances is ultimately unnecessary because websocket connexions regularly re-check their permissions for each command sent, whenever the 5-second cache expires.

Grr, I always have so much trouble with the github review interface. I can't find those new tests you mention.

As for logging in, Grist itself doesn't control whether users can log in or not (authentication). All I can handle on Grist's side is what to do with users once they've logged in (authorization). The solution here is that a disabled, logged-in user gets permission denied for almost everything.

As for logging in, I accept your conclusion, but I disagree that Grist doesn't control authentication. It accepts the sign-in claim when it calls ScopedSession's methods like updateUser or operateOnScopedSession in app/server/lib/GristLoginSystem.ts, core/app/server/lib/GristServer.ts, and core/app/server/lib/SamlConfig.ts. None of those places currently validate it against the database, which is why it's awkward to reject at those points. But it's definitely up to Grist if we wanted to be stricter.

dsagal · 2025-09-25T19:57:31Z

app/server/lib/Authorizer.ts

    }
  }

+  // In order to let a disabled user know that they're logged in and


How about moving this under if (disabledAt), since in nearly all cases this check doesn't need to happen?

dsagal · 2025-09-26T15:04:56Z

test/gen-server/ApiServerAccess.ts

      assert.deepEqual(resp.data.map((org: any) => org.name),
                                     ['Kiwiland', 'Fish', 'Flightless', 'Primately']);
+
+      // Check that a disabled user cannot use an api key


Two notes about these checks.

For one, they check requests to /api/orgs endpoint using API key. This would be better moved to it("prevents disabled users from logging in") -- which checks requests to other endpoints using API key. This particular test case is testing something about setting a new API key, and it's confusing to add unrelated checks here.

For another, it would be good to have a test case somewhere that uses a session cookie instead of the API key. For that you can use server.getCookieLogin(), and that check should also be in a test case specific to disabling users (could be the same it("prevents disabled users from logging in") one).

Okay, I've modified that test to include both token and cookie auth.

I need to write a test that does actually use sessions, and this environment variable is getting in the way. Let's move it out of the way.

A clear 403 is what they get if they try to log in again.

dsagal

Thank you. Looks good to me! (Reminder that this PR should still wait for Paul's review too.)

dsagal · 2025-09-27T05:34:22Z

test/server/lib/Authorizer.ts

+async function getChimpyCookie() {
+  const session = new TestSession(server);
+  return {
+    ... await session.getCookieLogin(


Is wrapping into {... X} needed here?

Sorry, vestige of a different approach. Gone now.

paulfitz · 2025-09-29T14:15:48Z

app/common/UserAPI.ts

  updateUserLocale(locale: string|null): Promise<void>;
  updateAllowGoogleLogin(allowGoogleLogin: boolean): Promise<void>;
+  disableUser(userId: number): Promise<void>;
+  enableUser(userId: number): Promise<void>


(missing semicolon?)

Huh, how did that get past the linter?

Fixed.

app/gen-server/lib/homedb/HomeDBManager.ts

app/gen-server/migration/1754077317821-UserDisabledAt.ts

paulfitz · 2025-09-29T14:27:52Z

app/gen-server/ApiServer.ts

+    const mreq = req as RequestWithLogin;
+    const userId = mreq.userId;
+    const targetUserId = integerParam(req.params.userId, 'userId');
+    if (targetUserId == userId) {


Please avoid ever using ==, favor ===.

Oops, don't know how that one slipped through.

paulfitz · 2025-09-29T14:32:03Z

app/server/lib/Authorizer.ts

+    );
+
+    if (!isSessionGetRequest) {
+      throw new ApiError('Forbidden', 403);


I think it would be ok to give a more explanatory error message. While other social products may attempt to not let a banned user know they are banned, I haven't seen a strong argument for Grist doing the same, and without that argument the general benefits of clarity win.

test/gen-server/ApiServer.ts

test/server/lib/Authorizer.ts

test/gen-server/ApiServer.ts

paulfitz · 2025-09-29T14:53:13Z

test/server/lib/GranularAccess.ts

+    assert.equal((await cliOwner.send("fetchTable", 0, 'Data')).data.tableData[3].A, 42);
+    assert.equal((await cliEditor.send("fetchTable", 0, 'Data')).data.tableData[3].A, 42);
+
+    // Large ham is the admin


Add potted explanation of "large ham" or just say "ham"

jordigh

@paulfitz made the requested changes

jordigh · 2025-09-29T15:32:14Z

app/common/UserAPI.ts

  updateUserLocale(locale: string|null): Promise<void>;
  updateAllowGoogleLogin(allowGoogleLogin: boolean): Promise<void>;
+  disableUser(userId: number): Promise<void>;
+  enableUser(userId: number): Promise<void>


Huh, how did that get past the linter?

Fixed.

app/gen-server/lib/homedb/HomeDBManager.ts

app/gen-server/migration/1754077317821-UserDisabledAt.ts

jordigh · 2025-09-29T16:12:51Z

app/gen-server/ApiServer.ts

+    const mreq = req as RequestWithLogin;
+    const userId = mreq.userId;
+    const targetUserId = integerParam(req.params.userId, 'userId');
+    if (targetUserId == userId) {


Oops, don't know how that one slipped through.

test/gen-server/ApiServer.ts

test/server/lib/Authorizer.ts

paulfitz

Thanks @jordigh !

jordigh force-pushed the jordigh/ban-user branch from 6caaa0a to 0f8fa8c Compare August 18, 2025 23:22

dsagal reviewed Aug 18, 2025

View reviewed changes

jordigh force-pushed the jordigh/ban-user branch from 0f8fa8c to 51cf32c Compare August 19, 2025 13:15

jordigh added 3 commits August 31, 2025 18:52

User: add a disabledAt property

a0b8e4c

This is in analogy with removedAt for documents. If the property is not null, then the user is considered disabled. To re-enable a user, we null the property again.

HomeDBManager: allow changing the disabledAt property of a user

7e02a23

Simply saving the new property to the db upon a change.

API: add /api/users/:userId/[enable|disable] endpoints

73eca92

These API endpoints allows disabling or enabling a user.

jordigh force-pushed the jordigh/ban-user branch 9 times, most recently from de5e603 to 71bc703 Compare September 3, 2025 01:10

jordigh force-pushed the jordigh/ban-user branch from 71bc703 to 718fc95 Compare September 3, 2025 01:22

jordigh changed the title ~~Backend changes for allowing enabling/disabling a user~~ API for enabling/disabling a user Sep 3, 2025

ApiServer: add tests for new user disable/enable endpoints

d01de21

Testing the API endpoints work and also a simple test that a banned user can't reach an endpoint via API.

jordigh marked this pull request as ready for review September 3, 2025 02:32

fflorent reviewed Sep 3, 2025

View reviewed changes

test/server/lib/Authorizer.ts Outdated Show resolved Hide resolved

app/gen-server/ApiServer.ts Outdated Show resolved Hide resolved

app/gen-server/ApiServer.ts Outdated Show resolved Hide resolved

fflorent reviewed Sep 3, 2025

View reviewed changes

app/gen-server/ApiServer.ts Outdated Show resolved Hide resolved

test/gen-server/ApiServer.ts Outdated Show resolved Hide resolved

test/server/lib/GranularAccess.ts Show resolved Hide resolved

fflorent approved these changes Sep 3, 2025

View reviewed changes

paulfitz reviewed Sep 10, 2025

View reviewed changes

dsagal reviewed Sep 22, 2025

View reviewed changes

dsagal reviewed Sep 23, 2025

View reviewed changes

app/gen-server/ApiServer.ts Show resolved Hide resolved

dsagal reviewed Sep 24, 2025

View reviewed changes

jordigh commented Sep 25, 2025

View reviewed changes

dsagal reviewed Sep 25, 2025

View reviewed changes

dsagal reviewed Sep 26, 2025

View reviewed changes

jordigh added 2 commits September 26, 2025 18:56

tests: disentangle env setup for Authorizer test

8474ff9

I need to write a test that does actually use sessions, and this environment variable is getting in the way. Let's move it out of the way.

Authorizer: disallow disabled users from logging in

dc01b7a

A clear 403 is what they get if they try to log in again.

dsagal approved these changes Sep 27, 2025

View reviewed changes

paulfitz reviewed Sep 29, 2025

View reviewed changes

jordigh commented Sep 29, 2025

View reviewed changes

paulfitz approved these changes Oct 1, 2025

View reviewed changes

jordigh force-pushed the jordigh/ban-user branch from e9af03f to d01de21 Compare October 1, 2025 19:34

jordigh merged commit 7b6b152 into main Oct 1, 2025
15 checks passed

jordigh deleted the jordigh/ban-user branch October 1, 2025 19:52

jordigh mentioned this pull request Oct 6, 2025

API for enabling/disabling a document #1865

Open

4 tasks

hexaltation mentioned this pull request Oct 7, 2025

Service Accounts API #1699

Open

4 tasks

	// Do not include removedAt field if it is not set. It is not relevant to regular
	// situations where the user is working with non-deleted resources.
	if (key === 'removedAt' && value === null) { return undefined; }
	// Don't bother sending option fields if there are no options set.
	if (key === 'options' && value === null) { return undefined; }
	// Don't prune anything that is explicitly allowed.
	if (allowedFields?.has(key)) { return value; }
	// User connect id is not used in regular configuration, so we remove it from the response, when
	// it's not filled.
	if (key === 'connectId' && value === null) { return undefined; }

Uh oh!

API for enabling/disabling a user #1775

API for enabling/disabling a user #1775

Uh oh!

Conversation

jordigh commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsagal left a comment

Choose a reason for hiding this comment

Uh oh!

jordigh commented Aug 19, 2025 • edited by dsagal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulfitz commented Aug 20, 2025

Uh oh!

jordigh commented Aug 20, 2025

Uh oh!

paulfitz commented Aug 21, 2025

Uh oh!

fflorent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fflorent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fflorent left a comment

Choose a reason for hiding this comment

Uh oh!

vviers commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulfitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dsagal left a comment

jordigh commented Aug 18, 2025 •

edited

Loading

jordigh commented Aug 19, 2025 •

edited by dsagal

Loading

vviers commented Sep 4, 2025 •

edited

Loading