feat: add support for mapping user admin status from OIDC claims - #1033
feat: add support for mapping user admin status from OIDC claims#1033UiP9AV6Y wants to merge 4 commits into
Conversation
1cda078 to
7c460e4
Compare
Some IdPs don't provide or only provide the claims in the ID token / userinfo. Most clients will check both claims.
Co-Authored-By: Jannis Mattheis <contact@jmattheis.de>
| switch roles := subject.(type) { | ||
| case string: | ||
| return r.resolveSlice([]string{roles}), nil | ||
| case []string: |
There was a problem hiding this comment.
This doesn't work. the groups are provided here as []any. (I've already adjusted this).
|
|
||
| // RBACResolver allows for arbitrary input to be mapped to roles for | ||
| // use in a permission system. | ||
| type RBACResolver interface { |
There was a problem hiding this comment.
I feel like this is pretty overengineered. All this matching and conversion should be done in a single function. This makes this IMO easier to understand. (I've already adjusted this)
7c460e4 to
20629d3
Compare
|
@eternal-flame-AD Could you take a look at these changes? I've made some larger adjustments and would like a second pair of eyes (: I've tested this with authentik and Authelia with different combinations of settings and groups. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1033 +/- ##
==========================================
+ Coverage 75.49% 75.75% +0.26%
==========================================
Files 66 66
Lines 3559 3622 +63
==========================================
+ Hits 2687 2744 +57
- Misses 662 667 +5
- Partials 210 211 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
sure, will do tonight :) |
| } | ||
|
|
||
| if user != nil { | ||
| if admin != nil && user.Admin != *admin { |
There was a problem hiding this comment.
The status of being not an admin is simply the lack of the admin role in the group claim, there isn't really a third "empty" state here IMO. Every case where there is not an admin role present is just not an admin.
Why not just make admin a boolean instead of tristate , and update the admin flag if GOTIFY_OIDC_GROUPS_ADMIN is defined. Ideally also check this configuration and prohibit manual admin changes in UpdateUserById()
The main risk I am worried about is accidentally omitting a code path where the admin status isn't stripped when it should be. I don't think there is really a use case where you might have this situation where you have some users have the GroupClaim but other don't.
There was a problem hiding this comment.
Why not just make admin a boolean instead of tristate , and update the admin flag if GOTIFY_OIDC_GROUPS_ADMIN is defined.
Okay
Ideally also check this configuration and prohibit manual admin changes in UpdateUserById()
I think I skip this for now, as we still allow logging in with user/pass for oidc accounts. I create a new ticket for this, as this is likely not that straightforward. => #1037
| # GOTIFY_OIDC_USERNAMECLAIM=preferred_username | ||
|
|
||
| # OIDC ID-token claim containing the group memberships as a list of strings. | ||
| # The claim must be part of the requested scopes (GOTIFY_OIDC_SCOPES). When |
There was a problem hiding this comment.
Should we check this or maybe just silently add it if it's not present?
There was a problem hiding this comment.
I'm not sure if we can. Afaik, this isn't standardized (https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.4) , so the claim-key could be something like "http://example.info/claims/groups" and the scope maybe too. Most IdPs use groups, but I rather not force this and instead document this per IdP in our OIDC documentation.
3 participants
an optional new configuration setting allows operators to define a JMESPath expression to calculate the admin status of a user based on their OIDC claims. this feature can also be used to deny users access alltogether, by mapping their claims to a special role value.
closes #957