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

feat: add access denied UI #27335

Merged
merged 4 commits into from
Jan 14, 2025
Merged

feat: add access denied UI #27335

merged 4 commits into from
Jan 14, 2025

Conversation

zlwaterfield
Copy link
Contributor

@zlwaterfield zlwaterfield commented Jan 7, 2025

Changes

Follow along on RBAC project here: #24512

This PR is waiting on: #27330

RBAC Access denied UI for supported resources: dashboards, insights, notebooks, feature flags. This will be loaded when the load resource request 403s. I looked at adding middleware to the API logic, but it was just too finicky and hard to determine when requests to intercept.

Note: this is not viewable by users yet because we aren't returning 403s yet unless they are using the new RBAC system.

Screenshot 2025-01-07 at 3 45 32 PM

Image will be updated, request here: PostHog/posthog.com#10285

👉 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

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Size Change: +9 B (0%)

Total Size: 1.13 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.13 MB +9 B (0%)

compressed-size-action

@zlwaterfield zlwaterfield requested a review from a team January 8, 2025 02:01
Copy link
Contributor

@joshsny joshsny left a comment

Choose a reason for hiding this comment

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

This will be a great addition 🔐

frontend/src/scenes/dashboard/dashboardLogic.tsx Outdated Show resolved Hide resolved
@@ -449,6 +450,7 @@ export const featureFlagLogic = kea<featureFlagLogicType>([
},
},
],
accessDeniedToFeatureFlag: [false, { setAccessDeniedToFeatureFlag: () => true }],
Copy link
Contributor

Choose a reason for hiding this comment

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

Do any of these cause a flashed screen before the endpoint returns, or are they all covered by page loaders?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah a bit, def room to improve.

frontend/src/scenes/insights/insightLogic.tsx Outdated Show resolved Hide resolved
@zlwaterfield zlwaterfield requested review from a team and Twixes January 9, 2025 14:31
Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Generally I don't like this approach as I'd love it to be more like how NotFound works in terms of somehow magically knowing that the access was denied rather than needing a lot of custom code. But its fine as a first step for sure.

@zlwaterfield
Copy link
Contributor Author

Generally I don't like this approach as I'd love it to be more like how NotFound works in terms of somehow magically knowing that the access was denied rather than needing a lot of custom code. But its fine as a first step for sure.

NotFound actually doesn't magically work (I thought it did too). I copied this pattern from NotFound because the request was too hard to intercept.

Screenshot 2025-01-09 at 12 05 08 PM

@benjackwhite
Copy link
Contributor

Generally I don't like this approach as I'd love it to be more like how NotFound works in terms of somehow magically knowing that the access was denied rather than needing a lot of custom code. But its fine as a first step for sure.

NotFound actually doesn't magically work (I thought it did too). I copied this pattern from NotFound because the request was too hard to intercept.

Yeah I mean this line though - auto detecting that there is a impersonation possibility. But I'm more thinking "later it would be good if we had a component you could use that intelligently understood common fetch outcomes". So not something for you but more for dev-ex

@@ -2742,7 +2742,7 @@ async function handleFetch(url: string, method: string, fetcher: () => Promise<R
error = e
}

apiStatusLogic.findMounted()?.actions.onApiResponse(response, error)
apiStatusLogic.findMounted()?.actions.onApiResponse(response?.clone(), error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed because it calls resonse.json() so downstream functions can't pull out the body response.

@zlwaterfield zlwaterfield merged commit d04b6ef into master Jan 14, 2025
99 checks passed
@zlwaterfield zlwaterfield deleted the zach/rbac/6b branch January 14, 2025 22:37
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