Simple but effective oauth discovery and userinfo endpoint #19934
Conversation
Signed-off-by: Juan Manuel Lallana <[email protected]>
Signed-off-by: Juan Manuel Lallana <[email protected]>
|
The continuous integration server fails, but for reasons that are completely unchanged from the change I made in the application. Does anyone follow these cases? |
philippstappert
left a comment
philippstappert
left a comment
There was a problem hiding this comment.
Don't know, why the check won't pass...
Checked the code on my own, works fine 👍
| ], | ||
| [ | ||
| 'name' => 'OauthApi#discovery', | ||
| 'url' => '/.well-known/openid-configuration', |
There was a problem hiding this comment.
well known urls need to be at the root of the domain I think.
There was a problem hiding this comment.
Nowhere is it specified that a subdirectory cannot be used as an identity provider. In fact keycloak as it has several realms uses the pattern https://{keycloak-domain}/auth/ realms/{realm-name}
There was a problem hiding this comment.
https://tools.ietf.org/html/rfc5785
If you define the route like above and Nextcloud is installed in a subdirectory the discovery endpoint would be https://nextcloud.test/subdirectory/.well-known/openid-configuration.
#16231 / nextcloud/documentation#1024 might be a starting point.
There was a problem hiding this comment.
The rewrite rules in .htaccess cannot influence paths outside of Nextcloud's subdirectory, either. So #16231 is equally affected by this problem. But apparently it's convention to register such routes with public.php instead of directly.
There was a problem hiding this comment.
Yep. To make this work for every setup we need to update nginx configuration and service discovery behind reverse proxy documentation. I would also prefer to add the redirect to .htaccess and pick a different name for the route (because it's like that for the other service discovery endpoints as well) but the current approach should also work.
It depends a bit how you read https://tools.ietf.org/html/rfc5785#section-1.1. I would say returning to openid configuration is fine here. But others might argue that we should redirect to a another route.
|
Discovery endpoint must be a public page
…On Mon, 23 Mar 2020 at 10:13, Roeland Jago Douma ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In apps/oauth2/lib/Controller/OauthApiController.php
<#19934 (comment)>:
> + * @NoCSRFRequired
+ *
+ * @return JSONResponse
+ */
+ public function discovery() {
+ $util = new Util();
+ return new JSONResponse([
+ 'issuer' => $this->urlGenerator->linkToRouteAbsolute(''),
+ 'authorization_endpoint' => $this->urlGenerator->linkToRouteAbsolute('oauth2.LoginRedirector.authorize'),
+ 'token_endpoint' => $this->urlGenerator->linkToRouteAbsolute('oauth2.OauthApi.getToken'),
+ 'userinfo_endpoint' => $this->urlGenerator->linkToRouteAbsolute('oauth2.OauthApi.getUserInfo')
+ ]);
+ }
+
+ /**
+ * @publicpage
This should not be a public page
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#19934 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4NCIPOXTJG3PNC2FSFL2LRI5N6BANCNFSM4LHEQRRA>
.
|
kesselb
left a comment
kesselb
left a comment
There was a problem hiding this comment.
Probably not bad to create a new controller only for the discovery logic.
| * @return JSONResponse | ||
| */ | ||
| public function discovery() { | ||
| $util = new Util(); |
| Throttler $throttler, | ||
| IUserSession $userSession, | ||
| IURLGenerator $urlGenerator) { |
There was a problem hiding this comment.
apps/oauth2/tests/Controller/OauthApiControllerTest.php needs an update for the new dependencies.
There was a problem hiding this comment.
Right, I fix it and resend the pull request
ChristophWurst
added
2. developing
yes. Small controller are happy controllers. |
|
After continuing with the changes in this application, I found that for openid connect to work it also needs changes in core. I gave up for the moment and I'm trying to make an application from scratch to give full support to openid connect. |
|
Any update on this? |
|
nope
…On Mon, 8 Nov 2021 at 19:37, JBM1866 ***@***.***> wrote:
Any update on this?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#19934 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4NCILN6FIMCXGBDYZ57KLULBGK7ANCNFSM4LHEQRRA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Would be nice to know if this will work in the near future. Apparently multiple oauth clients can't be integrated with NC in its current form. |
|
Was this supposed to also target RFC7591: OAuth 2.0 Dynamic Client Registration Protocol? |
|
As there is no feedback since a while I will close this ticket. Thanks for the interest in Nextcloud and the effort put into this! 🙇 |
10 participants
After continuing with the changes in this application, I found that for openid connect to work it also needs changes in core. I gave up for the moment and I'm trying to make an application from scratch to give full support to openid connect.