-
Notifications
You must be signed in to change notification settings - Fork 130
fix: add authorization for UI routes #917
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
base: main
Are you sure you want to change the base?
Conversation
Merges OIDC libary updates.
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #917 +/- ##
=======================================
Coverage 49.65% 49.65%
=======================================
Files 48 48
Lines 1718 1718
Branches 175 175
=======================================
Hits 853 853
Misses 841 841
Partials 24 24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thanks for the PR @jescalada and apologies for the delay! From a product perspective, I understand the position of closing down certain routes. My reason for opening them up to all users was to encourage "openness" when engaging with open source, and driving transparency on what projects are being contributed to and by whom. That said, I also appreciate the purpose of this change. Perhaps we make it configurable? |
@JamieSlome That makes sense! I'll make it optional. I'll modify #961, which contains the changes in this PR. |
Fixes #918.
This PR fixes the UI route access issue (unlogged users and non-admins can no longer access any route). In other words, only admins should be able to access the list of users, and only logged-in users can see the rest. Unlogged users can only access the login page.
You'll see an Unauthorized page when accessing a route you don't have access to.
Another issue we'd like to fix is the layout and the admin routes, which are coupled. Thus, all routes are called
/admin/*
when we only want the actual admin routes to have this naming (and replace regular routes with/dashboard/
). To keep things bite-sized, this will be pushed in a separate PR.It builds on top of the OIDC implementation at #905. As mentioned in that PR, testing with OIDC will require editing the config.
Tests will be modified/added in the layout fix PR since the routes will change.
Thank you!
Changelog
AuthProvider
PrivateRoute
NotAuthorized
andNotFound
pages