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

feat: restrict login to users matching a certain group #884

Merged
merged 8 commits into from
Nov 14, 2024

Conversation

bergerar
Copy link
Contributor

Fixes #867

Copy link

github-actions bot commented Jul 2, 2024

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@bergerar bergerar marked this pull request as ready for review July 22, 2024 14:07
@edward-ly
Copy link
Contributor

Closing as discussed in #867 (comment).

@edward-ly edward-ly closed this Oct 21, 2024
@Stefanqn
Copy link

I'm quite surprised about this decision: the referenced discussion describes a cumbersome workaround, including creating custom Keycloak flows and carefully adding the Keycloak plugin to multiple locations in these flows. Furthermore, it's an unofficial, single-developer plugin and it limits the solution space to Keycloak. On the other hand, a pull request with a proper solution at PEP (Policy Enforcement Point) exists and gets rejected. Would you please elaborate on this decision?

@edward-ly
Copy link
Contributor

edward-ly commented Oct 21, 2024

We happily welcome open discussion from anyone, not just those already mentioned. We're not saying that this feature isn't useful, but rather that it's better to implement it as a separate app or use LDAP as an alternative.

I agree that the Keycloak extension isn't a perfect solution, but what about my point above? What does this PR provide that another app doesn't (user_ldap or otherwise)? This is the first time I'm hearing about PEP as well, so please explain.

@Stefanqn
Copy link

Stefanqn commented Oct 24, 2024

Thank you Edward for keeping an open mind on this.

Regarding your point(s):
a) use LDAP as an alternative: I'm quite confused about this suggestion as it would result in switching to a completely different protocol for authorization - one many don't have in place. Maybe you can describe your idea in more detail.
b) implement it as a separate app: what would be the motivation to do so? You mentioned user_LDAP directly implementing this, so why do it differently for user_oidc?

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.

@edward-ly
Copy link
Contributor

Let's continue the discussion in #867 (comment) in which I made a response there.

@edward-ly
Copy link
Contributor

Reopening as per #867 (comment), will test and merge within the next few days.

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

@edward-ly edward-ly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please rebase the branch so that it starts at the tip of the main branch and resolve any conflicts? We'd like to test that it works on the latest version before merging, thank you.

@bergerar bergerar force-pushed the main branch 3 times, most recently from b397b76 to b3f4372 Compare October 29, 2024 21:49
@bergerar
Copy link
Contributor Author

I rebased the pull request, unit tests and lints are passing and I did some manual tests in my setup of the feature.

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane to me. Tested, works fine ✔️

A small improvement could be to check if the leading and trailing slashes are there and add them if not.

The labels/hints in the settings need to be as much descriptive as possible. See inline comments.

@bergerar
Copy link
Contributor Author

@julien-nc the delimiters are now added automatically if the first one is missing.

There was one check failing, but it was the same one as on the main branch (Integration tests / php8.1-mysql-stable27)

@julien-nc julien-nc self-requested a review November 13, 2024 13:42
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Could you run composer run cs:fix to make the lint/cs check pass?
I'll merge after that. We can ignore the failing integration tests. They fail because Php 8.3 is installed instead of 8.1 for some reason.

bergerar and others added 8 commits November 13, 2024 19:45
Signed-off-by: Armin Berger <contribute@arminberger.ch>
Signed-off-by: Armin Berger <contribute@arminberger.ch>
Signed-off-by: Armin Berger <contribute@arminberger.ch>
Signed-off-by: Armin Berger <contribute@arminberger.ch>
Signed-off-by: Armin Berger <contribute@arminberger.ch>
Co-authored-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Armin Berger <contribute@arminberger.ch>
Signed-off-by: Armin Berger <contribute@arminberger.ch>
Signed-off-by: Armin Berger <contribute@arminberger.ch>
@bergerar
Copy link
Contributor Author

bergerar commented Nov 13, 2024

I still had an old version of the php-cs-fixer.

I did now rebase, install the updated dependencies and run composer run cs:fix.

@julien-nc is should now pass all tests

@julien-nc julien-nc self-requested a review November 14, 2024 00:23
@julien-nc julien-nc merged commit d161cdd into nextcloud:main Nov 14, 2024
43 of 45 checks passed
@bergerar
Copy link
Contributor Author

Thanks for merging and your work in general on open source. @julien-nc

@julien-nc
Copy link
Member

@bergerar Thank you for your patience, your perseverance, your nice words and your contribution!

@julien-nc julien-nc mentioned this pull request Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restrict login to users matching a certain group
4 participants