Attach user from access token after checks #1614

guillett · 2025-05-14T10:20:59Z

Context and related issues

I built a custom widget to upload documents.

It relies on access tokens based requests.

Access token generated unauthenticated requests (logged in as anonymous) which leads to impossible attachment manipulation. (Anonymous user "made" the upload, but I want to add it to a row as user X, user X is not authorized, because of a check that limits who can access a yet-unreferenced attachment.)

To replicate the issue

have a table with an attachment column
have a row in that table
Use an access token (with write access) to add an attachment (and get an attachment id back from grist)
Use that same access token to link the new attachment to the row (try to add the id in the correct column)

1, 2 and 3 are ok but 4 fails. Well actually it fails when you are not the owner of the doc (because as a owner you have all the rights).

That's the reason why I would like the access token to authenticate as the user of the widget and therefore attach user information to the request in such a case.

(Many thanks to @fflorent)

To give more context, here is a working exemple. It highlights a slightly different issue.
https://grist.numerique.gouv.fr/o/docs/daZkkLK5fpoZ/Demo-limit-widget

https://docs.getgrist.com/c3nzPGK4VHbZ/Demo-limit-widget1

Here is the custom widget code.

I created this custom widget to show the difference between record addition via the widget and via the access token.

As an unlogged user both buttons generate record related to [email protected].
As a logged-in user records added with the widget are correcly linked but via the access token, records are linked to [email protected].

Proposed solution

Attach the user after verification that the access is legitimate at the document level.

Has this been tested?

👍 yes, I added tests to the test suite

guillett · 2025-05-14T11:45:42Z

test/server/lib/GranularAccess.ts

+    const tokenResult = (await cliEditor.send("getAccessToken", 0, {readOnly: false})).data;
+    const i7 = await postAttachment(editor, docId, tokenResult.token, "data7", "7.txt");
+    await assert.isFulfilled(getAttachment(owner, docId, i7));
+    //await assert.isFulfilled(getAttachment(editor, docId, i7));


This should be discussed, it fails because it is impossible to set maybeNew from the web api. To me, in should be made possible. I can open another PR for that.

Not sure of what you mean here, especially about the maybeNew. Can you provide more context please?

It was, to me, counterintuitive that as the editor uploader of an attachment I can't access it after upload.
For the attachment to be accessible to me, I need to add it to a row accessible to me (cf lines below this comment).

I refer to the maybeNew because it is linked to a specific permissive use case.

Yes this seems fairly straightforward to add support for, although I wonder what use-cases there would be apart from testing.

In addition to the commented out line, it would be good to add a comment explaining why it is commented out.

paulfitz · 2025-07-24T16:03:30Z

app/server/lib/Authorizer.ts

      }
+      const userId = tokenObj.userId;
+      const user = await dbManager.getUser(userId);
+      setRequestUser(mreq, dbManager, user!);


If, given an access token, you fully grant the request the identity of the user, it is important to check that this does not accidentally permit more than you intend.

For example, can you add a test showing that a request accessing /api/profile/apiKey using a token will be denied?

#1512 (comment)

I thought this is already tested with https://github.com/gristlabs/grist-core/blob/main/test/server/lib/AccessTokens.ts#L112-L118 (with a slightly different context/check).

Please indulge me? The family of endpoints having to do with documents and the endpoints that don't deal with documents operate quite differently. I see a check in

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

Lines 524 to 529 in fb65d72

if (tokenObj) {

// Sanity check: does the current document match the document the token is

// for? If not, fail.

if (!mreq.docAuth.docId || mreq.docAuth.docId !== tokenObj.docId) {

throw new ApiError('token misuse', 401);

}

that looks good in the context of documents. But what happens otherwise? What stops /api/profile/apiKey from returning a value once the user has been vouched for as your code does? Maybe there is something. It is worth a test. If it is already blocked, great, if not, you'd just need to reject use of an access token outside of the context of documents.

Sorry, I didn't mean not to do what is important here.

I wanted to add a test to ensure resquests outside of the document endpoint would fail but they actually do not generate responses with error code because of anonymous access. cf. https://github.com/gristlabs/grist-core/pull/1614/files#diff-1aeb508e4b5874641fe6a997ac27fa7315a9c05513aa0695bb889b9c5de4ca49R120

I added a test that ensure that the request is considered from a anonymous user and not the use linked to the provided token.

Would it is be enough for you? Let me know what I can add to this PR.

Hi @guillett, it all looks ok. I hadn't carefully checked where your intervention was placed.

test/server/lib/AccessTokens.ts

fflorent

From my prospective, LGTM

Co-authored-by: Florent <[email protected]>

paulfitz

Looks pretty good, thanks @guillett. One small request.

paulfitz · 2025-10-01T15:15:14Z

test/server/lib/GranularAccess.ts

+    const tokenResult = (await cliEditor.send("getAccessToken", 0, {readOnly: false})).data;
+    const i7 = await postAttachment(editor, docId, tokenResult.token, "data7", "7.txt");
+    await assert.isFulfilled(getAttachment(owner, docId, i7));
+    //await assert.isFulfilled(getAttachment(editor, docId, i7));


Yes this seems fairly straightforward to add support for, although I wonder what use-cases there would be apart from testing.

paulfitz · 2025-10-01T15:16:11Z

test/server/lib/GranularAccess.ts

+    const tokenResult = (await cliEditor.send("getAccessToken", 0, {readOnly: false})).data;
+    const i7 = await postAttachment(editor, docId, tokenResult.token, "data7", "7.txt");
+    await assert.isFulfilled(getAttachment(owner, docId, i7));
+    //await assert.isFulfilled(getAttachment(editor, docId, i7));


In addition to the commented out line, it would be good to add a comment explaining why it is commented out.

dsagal · 2025-10-01T20:44:15Z

app/server/lib/Authorizer.ts

      }
+      const userId = tokenObj.userId;
+      const user = await dbManager.getUser(userId);
+      setRequestUser(mreq, dbManager, user!);


I am uncomfortable with this setRequestUser call. It changes the request object, and makes it hard to know elsewhere which user the request object will contain. An accessToken should allow a limited scope to act as the given user, but so much code relies on the user in the request that it's hard to know what might unintentionally get authorized.

I need more time to look into this.

(One idea to look into is adding info for the effective user (user/userId/fullUser) as an extra property to mreq or to mreq.docAuth, and then update just the code that should be affected by accessToken to pay attention to that.)

I tried the last idea I mentioned (adding a property to mreq.docAuth) and it seems to work well, at least for the test cases in this PR: https://gist.github.com/dsagal/ebd7a0b5e8bce37caae4a335cf744caa.

What do you think? @paulfitz, interested in your feedback too.

Yes, using mreq.docAuth feels a clear way to do it.

guillett mentioned this pull request May 14, 2025

Fix attachment upload from custom widget with ACL #1512

Closed

4 tasks

guillett commented May 14, 2025

View reviewed changes

hexaltation added this to French administration Board May 14, 2025

hexaltation moved this to In Progress in French administration Board May 14, 2025

hexaltation requested a review from paulfitz June 11, 2025 14:18

hexaltation moved this from In Progress to Needs feedback in French administration Board Jun 11, 2025

hexaltation moved this from Needs feedback to Needs Internal Feedback in French administration Board Jun 18, 2025

guillett force-pushed the access-token-document-level-auth branch from 4a3f00b to ffe3068 Compare July 24, 2025 15:33

guillett mentioned this pull request Jul 24, 2025

Rephrase constant texts to improve readability in granularAccess tests about attachment #1613

Merged

4 tasks

paulfitz reviewed Jul 24, 2025

View reviewed changes

hexaltation moved this from Needs Internal Feedback to In Progress in French administration Board Aug 13, 2025

maluhialoha mentioned this pull request Sep 24, 2025

Allow to download attachment from custom widget link when ACL are applied #1853

Open

guillett requested a review from paulfitz September 24, 2025 14:30

guillett mentioned this pull request Sep 24, 2025

Fix accessToken generated in custom widgets using the "view as another user" feature #1594

Open

4 tasks

fflorent moved this from In Progress to Needs feedback in French administration Board Oct 1, 2025

fflorent moved this from Needs feedback to In Progress in French administration Board Oct 1, 2025

guillett added 7 commits October 1, 2025 13:21

Attach user from access token after checks

a231678

Lint

638e9fc

Adjust constant to convention

ece8399

fixup! Adjust constant to convention

4bdd5f8

Add test for access outside of docs

fb2eaf5

Lint

19b9851

Force CI

6db6701

guillett force-pushed the access-token-document-level-auth branch from 5a287e0 to 6db6701 Compare October 1, 2025 11:21

fflorent reviewed Oct 1, 2025

View reviewed changes

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

fflorent approved these changes Oct 1, 2025

View reviewed changes

hexaltation moved this from In Progress to Needs feedback in French administration Board Oct 1, 2025

Update test/server/lib/AccessTokens.ts

9b565bd

Co-authored-by: Florent <[email protected]>

paulfitz reviewed Oct 1, 2025

View reviewed changes

dsagal reviewed Oct 1, 2025

View reviewed changes

fflorent moved this from Needs feedback to In Progress in French administration Board Oct 22, 2025

	if (tokenObj) {
	// Sanity check: does the current document match the document the token is
	// for? If not, fail.
	if (!mreq.docAuth.docId \|\| mreq.docAuth.docId !== tokenObj.docId) {
	throw new ApiError('token misuse', 401);
	}

Uh oh!

Attach user from access token after checks #1614

Are you sure you want to change the base?

Attach user from access token after checks #1614

Uh oh!

Conversation

guillett commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context and related issues

Proposed solution

Has this been tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guillett Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

guillett Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

fflorent left a comment

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guillett commented May 14, 2025 •

edited

Loading

guillett Oct 1, 2025 •

edited

Loading

guillett Jul 24, 2025 •

edited

Loading