-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: client side access control checks #27635
base: master
Are you sure you want to change the base?
Conversation
fb9e3c8
to
580b445
Compare
Size Change: +766 B (+0.07%) Total Size: 1.16 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts on how this could be a little more scalable as it feels quite complex atm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, left a couple of comments
frontend/src/layout/navigation-3000/sidepanel/panels/access_control/accessControlLogic.ts
Outdated
Show resolved
Hide resolved
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
d776f6f
to
6510905
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
<AccessControlledLemonButton | ||
userAccessLevel={featureFlag.user_access_level} | ||
minAccessLevel="editor" | ||
resourceType="feature flag" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit(non-blocking): resourceType
is free text for inserting into the disabledReason
, but it's also passed to accessLevelSatisfied
where it needs to be correct. may be worth considering an enum or string literal
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Changes
Follow along: #24512
This PR is adding a component that checks if the current user can perform an action on a resource under the access control model and return a reason why if not. It will disable buttons/inputs where the user does not have access and show a tooltip on hover with more information. This includes a helped component named
<AccessControlledLemonButton />
that extendsLemonButton
to make it very easy to use.Initially adding for the main four resources (insights, dashboards, notebooks, feature flags). The actions being blocked are being made under the assumption the user will always have access to the underlying data ,so they will still be able to duplicate, view SQL, etc. They just won't be able to action edit or delete the resource. This doesn't yet cover 100% of cases but it's covering most. It's integrating into existing patterns of
canEditInsight
andcanEditDashboard
.Only relevant for those with the feature flag on.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Manually