Fix accessToken generated in custom widgets using the "view as another user" feature #1594
Conversation
@guillett are you seeing a case where granular access rules for a document are not applied when that document is accessed using access tokens? That would be very serious. I tried replicating but have not succeeded yet. |
|
@paulfitz sorry, I should I provided more context. I updated the title and initial comment. Here is the addition
|
|
The demo (but not really interesting because I didn't add your emails) |
|
To illustrate the issues on both #1594 and #1512 As an owner ([email protected] with userId = 27671)
Using ACL with an editor user ([email protected])
As anon user
|
Ok I think I understand now, you are talking about the View as another user feature, correct? Yes, it would be reasonable to extent that feature to cover getAccessToken calls from custom widgets. "Apply ACL" and "ACL views" could mean many things, it would be worth rephrasing. Then I think I see you reporting more limitations of the current implementation of getAccessToken, which is reasonable, it was added for one specific purpose and it could definitely do with some love to generalize it. |
There was a problem hiding this comment.
Thanks @guillett. Looks plausible. Can you add a test or tests for this? test/server/lib/AccessTokens.ts could be a good place. The "view as another user" feature interacts with the back end by adding a [email protected] query parameter.
This PR touches on security, so will need two reviewers. Once it has tests I'll recruit another reviewer.
|
hi @paulfitz thanks for your feedback on adding a test. I chose to add a test in |
| public async getAccessToken(docSession: OptDocSession, options: AccessTokenOptions): Promise<AccessTokenResult> { | ||
| const tokens = this._server.getAccessTokens(); | ||
| const userId = docSession.userId; | ||
| const user = await this._granularAccess.getUser(docSession); |
There was a problem hiding this comment.
This seems a reasonable improvement.
| } | ||
| const userId = tokenObj.userId; | ||
| const user = await dbManager.getUser(userId); | ||
| setRequestUser(mreq, dbManager, user!); |
There was a problem hiding this comment.
This looks like an unrelated change? And one that needs some testing to demonstrate that it hasn't introduced a weakness, as discussed in other PRs.
There was a problem hiding this comment.
If this is about uploading attachments, it would be good to link to an issue specifically about that.
Context
As a document owner I want to be able to check check how the document appears to other people.
ACL views is very useful for than.
Unfortunately, in a custom widget
grist.docApi.getAccessTokendoes not return a token for the impersonated user but for myself.With that PR, I can get an access token for the "good" user.
Broader context
Split #1512 into two PRs.
Two issues coexist :
But:
This PR is to solve 1. and leaves 2. to the original PR.
Proposed solution
Apply
_granularAccessHas this been tested?