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

Restrict login to users matching a certain group #867

Closed
bergerar opened this issue May 13, 2024 · 9 comments · Fixed by #884
Closed

Restrict login to users matching a certain group #867

bergerar opened this issue May 13, 2024 · 9 comments · Fixed by #884
Labels
enhancement New feature or request priority: normal

Comments

@bergerar
Copy link
Contributor

We would like to limit access to Nextcloud to users having certain groups. So when a user authenticates with OAuth, it should check the groups coming from the OAuth provider before a user is created / allowed to login in. If the user is not on a whitelist (regex), the user login is rejected.

Background

We have a member database containing many members from different divisions. Some divisions have their own Nextcloud instance, and it would be great to allow them to use the member database for login. But other divisions should not be able to use the Nextcloud instance. The user story is described in hitobito/hitobito_jubla#74 (german)

Proposal

  • Add a new field in the settings for setting a whitelist regex for which groups (gid) are synchronized with Nextcloud
  • Add a new checkbox in the settings "Restrict login for users without whitelisted groups"
  • During the authentication process, check if the user is in at least one group that is part of the whitelist
  • During the group synchronisation, only add and remove groups that are whitelisted. (this would also solve Group provisioning: delete only groups created by the plugin #866)

What do you think about this proposal?

@edward-ly
Copy link
Contributor

Hi, after a bit of research, I found this Keycloak extension app that might help solve your problem if you use Keycloak as your identity provider. Although, I've also seen some mixed opinions online regarding whether IdPs should handle user authorization as well (in addition to authentication), but I don't know whether or not that is something you care about. If you use an LDAP server, I believe the built-in user_ldap app already has this feature as well. I recommend you switch to one of those two options if you haven't already as we'd like to avoid duplicating features wherever possible.

If you still need more custom user authorization on the Nextcloud side, perhaps this feature could also be implemented as a completely separate Nextcloud app instead. Perhaps custom user groups/authorization might be useful in other contexts regardless of whether you use user_oidc or not. I know this would mean that restricted users/groups would exist in Nextcloud rather than elsewhere, but I think it's the more flexible approach.

@bergerar @julien-nc I'd love to hear what you think before making a decision on what to do next.

@edward-ly
Copy link
Contributor

Closing due to inactivity as well as some internal discussion, see the above recommendations.

@edward-ly edward-ly closed this as not planned Won't fix, can't repro, duplicate, stale Oct 21, 2024
@bergerar
Copy link
Contributor Author

bergerar commented Oct 22, 2024

Thanks for taking a look at my feature request.

We are using Hitobito as an IdP. It is a member database for big organizations and communities. In addition to performing the authentication, it does also provide the groups the user is part of. We would like to use these groups for the authorization in Nextcloud.

There are thousands of people on the same Hitobito instance, but the Nextcloud instance will only be shared with a smaller subset of the people. Only these people should have access to the Nextcloud instance. There are mechanisms to restrict user from using the Nextcloud instance (like setting to quote to 0), but having these users already logged in, still opens a lot of potential for security violations.

As we neither use Keycloak nor LDAP, these protocols would have to be implemented on the IdP side, which would require significant infrastructure changes. Therefore we would like to keep our OAuth setup. Having this feature also for OIDC and not only LDAP does not look like a duplicate feature to me, and more getting user_oidc more usable for different use cases.
Creating a separate Nextcloud app for custom user authorization could solve part of the issue. However, since the proposed feature tightly integrates with the user authentication and group synchronization process, we felt it made sense to implement it within the user_oidc plugin.

@edward-ly please consider reopening this issue

@edward-ly
Copy link
Contributor

edward-ly commented Oct 24, 2024

The reason I suggested refactoring the PR as a separate community app for this is that we at Nextcloud prefer to write apps that are modular rather than monolithic. OpenID Connect is only an authentication protocol, so it doesn't make sense for us to have user_oidc do anything other than authentication. Different IdPs may also support different authorization features or implement those features in different ways, which we cannot support as user_oidc needs to be IdP-agnostic. Merging the PR also means that we become responsible for supporting and maintaining the new feature for customers, which we don't have to resources to do at the moment.

We also disagree with the notion that this feature is "tightly integrated" with the app; we think the events emitted by user_oidc are enough for another app to use as a basis for developing custom OAuth authorizations. Another alternative would be to implement an extension for hitobito similar to Keycloak's; after all, if Keycloak can manage and filter out users on their end, there's nothing stopping other IdPs from implementing something similar as well. And just in case, if there are any extra users created by user_oidc that you want to delete, we now have an API endpoint where you can do that too (added in v6.1.0).

@Stefanqn
Copy link

Stefanqn commented Oct 24, 2024

OpenID Connect is only an authentication protocol

It is an authentication layer on top of the OAuth 2.0 authorization framework, see wikipedia.

Different IdPs may also support different authorization features or implement those features in different ways, which we cannot support as user_oidc needs to be IdP-agnostic.

the PR provides an IDP-agnostic feature

We also disagree with the notion that this feature is "tightly integrated" with the app

Would you mind explaining why it is already tightly integrated with user_ldap, yet for user_oidc it mustn't? It is a security feature specific to oidc and putting it in the oidc plugin already keeps nextcloud modular and aligns with the domain cuts. Forcing it outside would violate the anticipated domain cut from my point of view.

Another alternative would be to implement an extension for hitobito similar to Keycloak's; after all, if Keycloak can manage and filter out users on their end, there's nothing stopping other IdPs from implementing something similar as well.

Please read about PEP. The referred keycloak extension explicitly states it's not a PEP. Your suggestion wouldn't follow the OAuth concept.

@bergerar
Copy link
Contributor Author

Thanks for reconsidering.

Every feature adds some maintenance, but I hopped that by adding this feature into the app improve it as it is IdP generic. If you do not want to maintain this feature, I can understand it. The maintenance was also a big reason why I didn't want to create a separate app.

I will search for a different path, possibilities I see at the moment:

Nextcloud App that uses events emitted by user_oidc

  • TokenObtainedEvent is emitted when the raw JWT token is received. An event listener could decode the token on its own, check if the user is part of a whitelisted group and returns an empty token otherwise.
  • AttributeMappedEvent is emitted when getting the groups from the token. It could remove the not whitelisted groups and add groups of Nextcloud user is already part of (as they are otherwise removed)

This would be similar to the changes proposed in #884

Nextcloud App for importing the users directly from Hitobito
The users are imported regularly or by trigger by making an API call to Hitobito using a service token. The users can be created using the user_oidc API endpoint.

This would have the advantage that all users of a Hitobito group are already in Nextcloud and can be searched and so on. But changes in groups or roles might not be reflected immediately (unless combined with above).

Change Hitobito to allowing configuring this behavior
Maybe in the OAuth configuration, you can specify which groups are allowed to use this authenticator and also just return these groups. Or we could allow groups to add their own OAuth applications, but restrict the usage of these only to the members of this group.

This would have the advantage that this would also allow other OAuth application and not just Nextcloud to restrict which user can use the application.

@edward-ly
Copy link
Contributor

Would you mind explaining why it is already tightly integrated with user_ldap, yet for user_oidc it mustn't? It is a security feature specific to oidc and putting it in the oidc plugin already keeps nextcloud modular and aligns with the domain cuts. Forcing it outside would violate the anticipated domain cut from my point of view.

Perhaps we have different ideas on what "tight integration" or "domain cut" means, could you explain what those terms mean to you?

It is an authentication layer on top of the OAuth 2.0 authorization framework, see wikipedia.

Sorry, I forgot that OAuth is part of the specification too, so yes, it would make sense to use OAuth over LDAP in this case.

Please read about PEP. The referred keycloak extension explicitly states it's not a PEP. Your suggestion wouldn't follow the OAuth concept.

Please take a look at Keycloaks Authorization Service Architecture for Policy Enforcement Point. It means that e.g. Keycloak can define resource access, but the resource server (Nextclouds user_oidc) has to enforce it.

OK, so if I understand the guide correctly, the PDP (the logic to make a decision on whether to authorize a user or not) is the responsibility of the IdP, and PEP (the logic to grant or not grant access based on that decision) is the responsibility of the client. This sounds like a good design pattern to me, but it would also mean that this issue and PR will remain closed for violating this policy. Also, there are a few things still missing here:

  1. I couldn't find much information regarding where this "well-known authentication pattern" comes from. The best I could find is this NIST guide to secure web services, and I can't find this pattern mentioned anywhere in the OIDC or OAuth specifications or another standard. If you can provide links to additional information, that would help us determine whether this is a standard worth implementing.
  2. The PEP libraries that Keycloak provides to actually enforce that policy only work with Keycloak, which is not what we want as previously discussed. If you know of any PEP implementation libraries that are also IdP-agnostic, and/or have an outline on how PEP might be implemented in user_oidc, that would also help us determine the feasibility of this feature.

Change Hitobito to allowing configuring this behavior Maybe in the OAuth configuration, you can specify which groups are allowed to use this authenticator and also just return these groups. Or we could allow groups to add their own OAuth applications, but restrict the usage of these only to the members of this group.

This would have the advantage that this would also allow other OAuth application and not just Nextcloud to restrict which user can use the application.

This sounds similar to my new suggestion which is to implement PAP/PDP in hitobito. That would also free up user_oidc so that it only needs to sync the groups listed in the ID token (as it normally does) for PEP-authorized users only.

@edward-ly
Copy link
Contributor

After some more internal discussion with management, we decided that we will accept and merge the PR soon after all, as it won't interfere with the potential new solution and the changes are small enough for us to manage in the meantime. Also, we have some doubts regarding how "well-known" the above design pattern actually is if not every IdP has implemented it, but we can continue this discussion here or in a separate issue as needed.

@edward-ly edward-ly reopened this Oct 25, 2024
@rriemann
Copy link

Dears,

The public sector organisation I work relies on "EU Login" as SSO, which is quite popular amongst EU institutions and does not use keycloak. "EU Login" is used by several thousand applications and the EU Login owners insist that authorization has to be handled by the business owner of the application and not the SSO. This led eventually to this code change in the user_saml plugin:

nextcloud/user_saml#670

I understand that user_oidc offers nearly equivalent functionality to user_saml with a more modern protocol. I hope that the pull request #884 can be integrated to close the gap further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: normal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants