Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make access control for ConvertFromColumn action less brutal #1111

Merged
merged 2 commits into from
Jul 24, 2024

Conversation

paulfitz
Copy link
Member

@paulfitz paulfitz commented Jul 17, 2024

Access control for ConvertFromColumn in the presence of access rules had previously been left as a TODO. This change allows the action when the user has schema rights. Because schema rights let you create formulas, they let you read anything, so there is currently no value in nuance here.

@paulfitz paulfitz force-pushed the paulfitz/convert-column-acl branch from d65275f to 2c7af80 Compare July 18, 2024 02:04
@paulfitz paulfitz marked this pull request as ready for review July 19, 2024 15:43
Access control for ConvertFromColumn in the presence of access rules
had been left as a TODO. This change allows the action when the user
has broad rights to the table it applies to.
@paulfitz paulfitz force-pushed the paulfitz/convert-column-acl branch from 8c1acac to 6e00834 Compare July 19, 2024 20:36
@georgegevoian georgegevoian self-requested a review July 22, 2024 14:11
@berhalak berhalak self-requested a review July 23, 2024 13:05
// If changes could include Python formulas, then user must have
// +S before we even consider passing these to the data engine.
// Since we don't track rule or schema changes at this stage, we
// approximate with the user's access rights at beginning of
// bundle.
// We also check for +S in scenarios that are hard to break down
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point out what scenarios you have in mind here? I think mentioning the Copy/Transform action would be enough.

{id: 'C', type: 'Int'}]],
['AddRecord', '_grist_ACLResources', -1, {tableId: 'Table1', colIds: 'C'}],
['AddRecord', '_grist_ACLRules', null, {
resource: -1, aclFormula: 'user.Access == OWNER', permissionsText: '-R',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you add this rule, I don't think it is exersiced anywhere below?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need at least one rule. In the absence of any rules, SPECIAL_ACTIONS are already allowed for editors, so the test would succeed trivially.

Added a comment.

@paulfitz paulfitz requested a review from berhalak July 24, 2024 14:46
Copy link
Contributor

@berhalak berhalak left a comment

Choose a reason for hiding this comment

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

Thanks.

@paulfitz paulfitz merged commit fc3a7f5 into main Jul 24, 2024
11 checks passed
@paulfitz paulfitz deleted the paulfitz/convert-column-acl branch July 24, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants