feat: client side access control (WIP) #27174
Open
+620
−310
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zlwaterfield
commented
Dec 27, 2024
| @@ -46,6 +61,13 @@ export const apiStatusLogic = kea<apiStatusLogicType>([ | |||
| const data = await response?.json() | |||
| if (data.detail === 'This action requires you to be recently authenticated.') { | |||
| actions.setTimeSensitiveAuthenticationRequired(true) | |||
| } else if (data.code === 'permission_denied') { | |||
| // TODO - only do if the RBAC feature flag is enabled | |||
There was a problem hiding this comment.
importing feature flag logic here is breaking this logic so haven't figured out this check yet
zlwaterfield
commented
Dec 27, 2024
| if (featureFlags[FEATURE_FLAGS.ROLE_BASED_ACCESS_CONTROL]) { | ||
| return true | ||
| // TODO: figure out how to access this method | ||
| // return hasResourceAccess({ userAccessLevel: dashboard.user_access_level, requiredLevels: ['admin', 'editor'] }) |
There was a problem hiding this comment.
Need to figure out how to access access control logic hasResourceAccess
zlwaterfield
commented
Dec 27, 2024
| if (featureFlags[FEATURE_FLAGS.ROLE_BASED_ACCESS_CONTROL]) { | ||
| return true | ||
| // TODO: figure out how to access this method | ||
| // return hasResourceAccess({ userAccessLevel: insight.user_access_level, requiredLevels: ['admin', 'editor'] }) |
There was a problem hiding this comment.
Need to figure out how to access access control logic hasResourceAccess
zlwaterfield
requested review from
aspicer,
Twixes,
dmarticus,
daibhin,
neilkakkar,
EDsCODE,
benHPostHog and
benjackwhite
and removed request for
benHPostHog
|
Size Change: +547 B (+0.05%) Total Size: 1.11 MB ℹ️ View Unchanged
|
zlwaterfield
changed the title
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
(this is still a WIP but wanted feedback on the implementation so far)
Follow along with all RBAC changes here: #24512
This PR is starting to layer on the client side access control changes. This PR is trying to layer on these changes without affecting this current experience. The main goal is to block access to actions where the user does not have access (editing, renaming, deleting, etc.). The API handles all this but we want the client to properly render what actions the user has access to.
It includes
AccessControlActionwrapper component to make the experience consistent for showing the user doesn't have access - takes in required levels and current levels and returns a message if they don't have access and whyhasResourceAccessmethod in the access control logic that theAccessControlActionuses to determine accessTODO:
canEditInsightandcanEditDashboardworkingcan_editand make sure this doesn't break anything + works as expected going forwardNote: this has not been implemented for projects, teams, or environments - it's focused on resources.
Feedback
Asking all leads for reviews as this will affect all teams once implemented. Please provide thoughts/feedback on the way this implemented. This is attempting to make a reuseable pattern to it's easy to add access level checks.
An alternative approach could be to return an object from
hasResourceAccesswhich has the disabled reason and the wrapper is not needed.👉 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 testing + will add some snapshots