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(oauth2): simple userinfo endpoint #43684

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

hardviper
Copy link

@hardviper hardviper commented Feb 20, 2024

Summary

This solves the oAuth2 authorization issue in openproject when openproject is the client.
Added endpoint that returns a minimum set of user information.
This is very necessary in my openproject integration with nextcloud.
I hope this helps someone else.
Thanks to @jlallana for suggesting this solution in PR #19934.

Checklist

@hardviper hardviper marked this pull request as draft February 20, 2024 12:26
@hardviper hardviper marked this pull request as ready for review February 20, 2024 12:26
@skjnldsv skjnldsv added the 2. developing Work in progress label Feb 21, 2024
@hardviper
Copy link
Author

@skjnldsv Is there anything else I can add to help you accept this?

@skjnldsv
Copy link
Member

@skjnldsv Is there anything else I can add to help you accept this?

I pinged the proper devs, let's see :)
Does this supersede #19934 ?

@hardviper
Copy link
Author

@skjnldsv This adds the user_info endpoint, but does not add the discovery.
Now I have fixed the problem noted by the bot, and also added some more userinfo fields.

Signed-off-by: d.kudrinskiy <hardviper@icloud.com>
@skjnldsv
Copy link
Member

There was an issue with the rebase, please fix it

Signed-off-by: d.kudrinskiy <hardviper@icloud.com>
Signed-off-by: d.kudrinskiy <hardviper@icloud.com>
@hardviper
Copy link
Author

@skjnldsv Ready! I have corrected! I don't understand how these commits got here.

Signed-off-by: d.kudrinskiy <hardviper@icloud.com>
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

I'm fine with the endpoint. Some instructions for testing would be appreciated.

The name split logic probably doesn't work for all names. If we can I would strongly favor removing it and only exposing the full name.

apps/oauth2/lib/Controller/OauthApiController.php Outdated Show resolved Hide resolved
apps/oauth2/lib/Controller/OauthApiController.php Outdated Show resolved Hide resolved
apps/oauth2/lib/Controller/OauthApiController.php Outdated Show resolved Hide resolved
apps/oauth2/lib/Controller/OauthApiController.php Outdated Show resolved Hide resolved
apps/oauth2/lib/Controller/OauthApiController.php Outdated Show resolved Hide resolved
apps/oauth2/lib/Controller/OauthApiController.php Outdated Show resolved Hide resolved
apps/oauth2/lib/Controller/OauthApiController.php Outdated Show resolved Hide resolved
apps/oauth2/tests/Controller/OauthApiControllerTest.php Outdated Show resolved Hide resolved
apps/oauth2/tests/Controller/OauthApiControllerTest.php Outdated Show resolved Hide resolved
config/config.sample.php Outdated Show resolved Hide resolved
hardviper and others added 5 commits March 13, 2024 21:50
Co-authored-by: Christoph Wurst <ChristophWurst@users.noreply.github.com>
Signed-off-by: Danila <57199291+hardviper@users.noreply.github.com>
Co-authored-by: Christoph Wurst <ChristophWurst@users.noreply.github.com>
Signed-off-by: Danila <57199291+hardviper@users.noreply.github.com>
Co-authored-by: Christoph Wurst <ChristophWurst@users.noreply.github.com>
Signed-off-by: Danila <57199291+hardviper@users.noreply.github.com>
Co-authored-by: Christoph Wurst <ChristophWurst@users.noreply.github.com>
Signed-off-by: Danila <57199291+hardviper@users.noreply.github.com>
Co-authored-by: Christoph Wurst <ChristophWurst@users.noreply.github.com>
Signed-off-by: Danila <57199291+hardviper@users.noreply.github.com>
Signed-off-by: d.kudrinskiy <hardviper@icloud.com>
@kesselb
Copy link
Contributor

kesselb commented Mar 13, 2024

Thanks for your pull request 👍

I'm against the process_name feature even as optional. Once merged, we have to maintain and support it. Furthermore, we cannot just drop the feature when people started using it and rely on it. But the approach is just wrong, and a similar approach in CardDAV (Converter.splitFullName) already caused trouble.

@Altahrim Altahrim mentioned this pull request Mar 14, 2024
@hardviper
Copy link
Author

@kesselb Thank you for your time and feedback on my pull request!
I understand your concerns about supporting the new feature and I want to discuss a few points.
Endpoint was developed by me to integrate with OpenProject, without name separation, my integration does not work. Name separation expands the capabilities and improves the overall functionality of nextcloud. There are probably OAuth2 clients that also necessarily need separated names. The result that my code returns is consistent with the OpenID specification. The functionality that causes your concerns is additional and does not affect the existing functionality. Users can choose whether they want to use this feature or not. I am ready to support and develop this feature after its implementation. The code is covered with tests, this will help to avoid problems and simplify the support of the function in the future.
I looked at the CardDAV code (Converter.splitFullName), in my opinion, name separation is implemented much more flexibly and fault-tolerant here. Tell us what problems you had to face with CardDAV (Converter.splitFullName)? Perhaps I can add something here that will save my code from the problems that you have encountered? I am open to further discussions and ready to make any necessary changes.

This was referenced Mar 18, 2024
@hardviper
Copy link
Author

@julien-nc @kesselb @ChristophWurst @skjnldsv I suggest we return to the discussion of PR. Is there anything else I can do to convince you to accept my contribution?

@kesselb
Copy link
Contributor

kesselb commented Mar 22, 2024

The approach to split the full name by a given character assumes that every name follows the same pattern.

Please look at https://en.wikipedia.org/wiki/Personal_name to get a better understanding how different personal names are.

To solve this problem, we need our own fields to store first name/last name and make the full name a getter from them.

I assume you can still implement this endpoint via your own Nextcloud app.

@Altahrim Altahrim mentioned this pull request Mar 25, 2024
@hardviper
Copy link
Author

hardviper commented Mar 25, 2024

To solve this problem, we need our own fields to store first name/last name and make the full name a getter from them.

@kesselb Is there an addition of last name and first name fields in the roadmap?

@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@kesselb
Copy link
Contributor

kesselb commented Mar 28, 2024

Is there an addition of last name and first name fields in the roadmap?

Forwarded your question.
Please ping me if we haven't replied within the next 4 weeks.

@hardviper
Copy link
Author

@kesselb Do you have an answer for the roadmap?

@kesselb
Copy link
Contributor

kesselb commented Jun 3, 2024

Do you have an answer for the roadmap?

Having fields for first name, last name, title or prefix as addition/replacement for the full name field is something we want.

However, it's not scheduled for the current or next iteration.

https://github.com/H2CK/oidc could be interesting for you as well.

This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants