Add oidc_login_allowed_roles

[clauso]

We use groups to control what shared folders users should be able to access. We use a role to control which users are allowed to login to nextcloud. So the 'oidc_login_allowed_groups' feature is not usable for use. In this pull request I added a 'oidc_login_allowed_roles' feature, which works with roles.

[azmeuk]

The role claim is not part of the OIDC specification, so I don't think we should have specific code for non-standard claims. However, the group claim is not part of the OIDC spec either, although it is commonly used.

I think a good solution to solve all this would be to have a more generic implementation of oidc_login_allowed_groups. I suggest removing oidc_login_allowed_groups, and add two new parameters: oidc_login_filter_claim_name, that could have the value of groups or roles or any other claim, and oidc_login_filter_claim_value that would be the actual expected groups or roles. (Though I am not sure for the names). @pulsejet @sirkrypt0 what do you think?

[clauso]

Just a quick thought on your suggestion: renaming oidc_login_allowed_groups would break the setup of users, that rely on it. Suggestion: I could rename my new option to oidc_login_filter_claim_value. Then you could mark the group variant as deprecated and remove it after a longer while?

[clauso]

Hi, I would really like to push this forward as I need this in our production and I am willing to adjust the code as needed. I would be very happy if we could find an agreement on the needed changes quickly so I can get to it. @pulsejet @sirkrypt0 could you please comment? Thanks!

[clauso]

@azmeuk Is there anything I can do to get this moving forward?

[azmeuk]

Hi. Not from my side. As I said I am in favor of a more generic solution but I wont take the decision alone. We need to wait for the other maintainers advices. As this is a volunteer project, this may take time depending on the their current occupations.

[clauso]

Another week has passed. @azmeuk could you just take the decision? I will gladly put some more time in this feature, if it needs adjustment to be merged. I like your suggestion, I just need a decision on how we proceed. Thanks.

[pulsejet]

Overall I think this is fine. I'm only worried about the terminology here ("groups" and "roles" doesn't seem very intuitive).

Why not just (for example) allow multiple fields for groups (so all roles and groups are just groups), then add a config option to check if the user is in some group before allowing login?

[pulsejet]

Correction, we already have oidc_login_allowed_groups, so unless I missed something all that's needed is allowing multiple fields for groups.

[azmeuk]

Actually what @clauso needs is a filter on a different claim than groups, that is roles. My suggestion was to make the claim to look at for user authorization more generic.

[pulsejet]

Actually what @clauso needs is a filter on a different claim than groups, that is roles. My suggestion was to make the claim to look at for user authorization more generic.

Right. So to rephrase my question, is it okay if we just merge the two (groups and roles), then check membership on the merged array. As long as the groups and roles have no overlap, this should be equivalent without making the config more complex than it already is.

[clauso]

@pulsejet thanks for your comments. Two thoughts on that:

a) The term "roles" is not needed, you are right, we just need some claim that is the login filter. However groups are required, as groups is a concept used in nextcloud for sharing files. That's how I initially hit the problem: If we use groups for the access part, nextcloud will create a lot of groups for all the roles etc. So I don't think it is a good idea to mix these. Groups are needed for sharing and something like a role ist needed to decide if a user can access nextcloud.

b) merging groups and roles does not sound like a good idea: Allowing access is a security matter and with security you want things to be very clear, transparent and obvious. Things like "as long as the groups and roles have no overlap" seem like this will sooner or later surprise some admins/users.

I think @azmeuk made a very good suggesting in comment https://github.com/pulsejet/nextcloud-oidc-login/pull/217#issuecomment-1432016475 : Keep groups as they are required for sharing in nextcloud and have another option for the login filter as these are two different things. (And I would mark the oidc_login_allowed_groups as deprecated then)

[clauso]

@pulsejet do you agree to go with azmeuk's and my last suggestion? "I think a good solution to solve all this would be to have a more generic implementation of oidc_login_allowed_groups. I suggest removing oidc_login_allowed_groups, and add two new parameters: oidc_login_filter_claim_name, that could have the value of groups or roles or any other claim, and oidc_login_filter_claim_value that would be the actual expected groups or roles." I am just waiting for your "go" before I make the change.

[pulsejet]

Okay, that sounds good. We'll need a new major version because of the breaking change.

Note that the returned value from the oidc_login_filter_claim_name can either be an array or space separated string (similar to groups). Maybe reuse some code there.

[clauso]

@pulsejet Thanks for following up. I can implement it quickly. I will keep the old allowed_groups and mark them as deprecated in the documentation. This allows us to introduce the new feature right away without a major version and do the major version whenever we decide to remove allowed_groups. I will update the pull request in the next days and it will be clear what I mean.

[clauso]

Currently working on it. I will call it login_filter in the attribute map. This could be (for keycloak)

'login_filter' => 'realm_access_roles'

The option should be

'oidc_login_filter_allowed_values' => array('role1', 'group2', 'whatever3')

This is in line with the other current naming of attributes and options. Ok?

[pulsejet]

Sounds okay to me

[clauso]

Done making the change. Looking forward to getting this merged soon. Please let me now if I can help with anything/answer any questions.