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 2 commits into
Conversation
|
|
||
| var roleExpression jmespath.JMESPath | ||
| if conf.OIDC.RoleExpression != "" { | ||
| roleExpression, err = jmespath.Compile(conf.OIDC.RoleExpression) |
There was a problem hiding this comment.
I don't think we need a full expression language here. Instead I propose three settings like this:
# OIDC ID-token claim containing the user's group memberships as a list of
# strings.
#
# Type: text
# GOTIFY_OIDC_GROUPS_CLAIM=groups
# Groups allowed to log in via OIDC. When set, users whose groups claim
# matches neither USER_GROUPS nor ADMIN_GROUPS are rejected. When empty,
# every user authenticated by the identity provider may log in.
#
# Type: text-list
# Example: messenger,gotify
# GOTIFY_OIDC_USER_GROUPS=
# OIDC groups whose members are granted admin rights.
# Membership is evaluated after each login.
#
# Type: text-list
# Example: admins,gotify-admins
# GOTIFY_OIDC_ADMIN_GROUPS=
There was a problem hiding this comment.
I don't think we need a full expression language here.
does that mean you are waiting for a second opinion or is that decision final? i am fine with the proposed changes, but wanted to clarify before i implement them.
There was a problem hiding this comment.
GOTIFY_OIDC_USER_GROUPS
GOTIFY_OIDC_ADMIN_GROUPS
shouldn't that be GOTIFY_OIDC_GROUPS_USER_ROLE and GOTIFY_OIDC_GROUPS_ADMIN_ROLE to follow the nested hierarchy nature of the config keys?
in v2 this would have looked like this:
oidc:
groups:
claim: groups
user_role: [messenger, gotify]
admin_role: [admins, gotify-admins]There was a problem hiding this comment.
does that mean you are waiting for a second opinion or is that decision final? i am fine with the proposed changes, but wanted to clarify before i implement them.
It's not final, you can tell your opinion, and give good points why a expression language is needed here. But if the three-setting solution is good enough that I'd prefer this.
shouldn't that be GOTIFY_OIDC_GROUPS_USER_ROLE and GOTIFY_OIDC_GROUPS_ADMIN_ROLE to follow the nested hierarchy nature of the config keys?
Good point, but I dislike the group and role in one name, as it basically describe the same thing. How about
GOTIFY_OIDC_GROUPS_CLAIM
GOTIFY_OIDC_GROUPS_USER
GOTIFY_OIDC_GROUPS_ADMIN
?
There was a problem hiding this comment.
i have no strong opinion about using an expression languange. i have simply chosen this approach, as Grafana does the same. my personal deployment would also work with the simplified approach. this approach is also what other applications (e.g. ArgoCD) use for their mapping.
i have pushed a new commit with the refactored system. test and documentation have also been updated.
21111f3 to
1cda078
Compare
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 gotify#957
instead of running the OIDC claims against a generic expression evaluator (like Grafana does), we simple calculate the user roles based on the group membership. this is similar to how ArgoCD, ElasticSearch, and other perform their permission mapping.
1cda078 to
7c460e4
Compare
2 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