Skip to content

Conversation

guillett
Copy link
Contributor

@guillett guillett commented Mar 14, 2025

Context

I am building a custom widget to sign PDF (not electronically, yet?) and I had issues when access rules apply.

Proposed solution

Fixing auth related bugs. 😬

Related issues

First of all, it was hard to get a minimal failing test.
To get a working bogus example

  • Create a new doc
  • Update column A to be of attachment type
  • Add an editor
  • Add an ACL rule to prevent editors from editing
  • Add a table specific ACL rule to allow editors edition
  • Add a custom widget to your view (for instance https://betagouv.github.io/grist-previsionnel/signer-pdf)
    • (link it to Table1 to select)
  • Create a new row adding a PDF in column A
  • It should display the PDF document in the custom widget
  • Click on « create and add updated PDF »
  • Nothing should happen
  • In the browser console, the « Error: Cannot access attachment » error should appeared.

image

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this is not relevant here
  • 🙋 no, because I need help I don't really know where to look at to add a test (or multiple)

Screenshots / Screencasts

@fflorent fflorent self-requested a review March 25, 2025 21:16
@guillett
Copy link
Contributor Author

guillett commented Apr 7, 2025

Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @guillett. Some questions to help my understanding.

mreq.accessToken = token;
// Once an accessToken is supplied, we don't consider anything else.
// User is treated as anonymous apart from having an accessToken.
mreq.user = user;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe? Could this potentially grant access to material beyond the specific document?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that, this is safe as long as the party with access to the provided token is trusted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I understand the concern you raise with that question. It may be possible to access (or update) a another document from a custom widget linked to a document with read (and write) access.

I have the feeling that if this fix "only" reinforce that vulnerability but it does not create it.

I looked at the existing test suite, I remember some related to access limitations but I can't remember if any exist on cross document access limitation/validation.

Copy link
Contributor Author

@guillett guillett Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I tested this and I get a token misuse from app/server/lib/Authorizer.ts. Tested on DINUM sandbox environment and with the fix on my computer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repository with tests https://github.com/guillett/test-grist-access/blob/main/pages/index.js

(Sorry I did not added a test in grist-core test suite)

this._pruneColumns(action, permInfo, tableId, accessCheck);
}

private async _checkAttachmentAccess(docSession: OptDocSession, attId: number) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you talk me through what this fixes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fixes one specific situation. When a file is not attached to any cell yet. Without this fix, the access is denied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the lazy && does the same thing 😬

public async getAccessToken(docSession: OptDocSession, options: AccessTokenOptions): Promise<AccessTokenResult> {
const tokens = this._server.getAccessTokens();
const userId = getUserId(docSession);
const user = await this._granularAccess.getUser(docSession);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you walk me through what this fixes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It applied ACL related accessses instead of connected user's.

@fflorent
Copy link
Collaborator

fflorent commented Apr 7, 2025

Also @guillett, I have just tried your reproduction scenario without success. I wondered if there exist steps that I should follow as the editor instead of the owner.

@guillett
Copy link
Contributor Author

guillett commented Apr 7, 2025

@fflorent here a quick/fast 😅 demo
Capture vidéo du 2025-04-07 17-21-11-s.webm

@guillett guillett closed this Apr 7, 2025
@fflorent
Copy link
Collaborator

fflorent commented Apr 7, 2025

@guillett thanks. I wonder if that's not due to the "view as" feature, simply. I'll try to reproduce that on my side.

Also I see that you closed the issue, is it a missclick?

@guillett guillett reopened this Apr 7, 2025
@guillett
Copy link
Contributor Author

guillett commented Apr 7, 2025

Oh yes a missclick sorrryy

@guillett
Copy link
Contributor Author

guillett commented Apr 7, 2025

@fflorent I confirm I have the issue when adding an editor (on another browser) and trying to sign and upload a file.

@fflorent
Copy link
Collaborator

fflorent commented Apr 8, 2025

Ah, I have made a mistake in my access rules. I know reproduce your issue @guillett!

Also I could locate the code of your widget: https://github.com/betagouv/grist-previsionnel/blob/main/pages/signer-pdf.js

@fflorent
Copy link
Collaborator

fflorent commented Apr 8, 2025

Interestingly, to reproduce the issue, the editor MUST be removed the READ access to all the tables by default, which is the most important part:

Table rules with editors forbidden to READ records on all tables by default, but granted update, creation and deletion

If they are granted this access, the error does not occur anymore, even when they are forbidden update, creation and deletion rights to records by default (overridden by the specific rules for Table1):
Access rules with the editor granted read access by default for all the table

@fflorent fflorent moved this to Needs feedback in French administration Board Apr 9, 2025
@fflorent fflorent added this to