Skip to content

Simple but effective oauth discovery and userinfo endpoint #19934

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.

Sign up for GitHub

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

Closed
jlallana wants to merge 2 commits into from
Closed

Simple but effective oauth discovery and userinfo endpoint #19934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c7afae
Simple but effective oauth discovery and userinfo endpoint
jlallana Mar 13, 2020
dbb0229
Issuer in discovery json
jlallana Mar 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions apps/oauth2/appinfo/routes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,15 @@
'url' => '/api/v1/token',
'verb' => 'POST'
],
[
'name' => 'OauthApi#discovery',
'url' => '/.well-known/openid-configuration',
Copy link
Member

@rullzer rullzer Mar 23, 2020

Choose a reason for hiding this comment

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

well known urls need to be at the root of the domain I think.

Copy link
Author

@jlallana jlallana Mar 27, 2020

Choose a reason for hiding this comment

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

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}

Copy link
Contributor

@kesselb kesselb Mar 27, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jplitza jplitza Jun 24, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@kesselb kesselb Jun 24, 2020

Choose a reason for hiding this comment

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

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.

'verb' => 'GET',
],
[
'name' => 'OauthApi#getUserInfo',
'url' => '/api/v1/userinfo',
'verb' => 'GET'
],
],
];
44 changes: 43 additions & 1 deletion apps/oauth2/lib/Controller/OauthApiController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@
use OCP\IRequest;
use OCP\Security\ICrypto;
use OCP\Security\ISecureRandom;
use OCP\Util;
use OCP\IURLGenerator;
use OCP\IUserSession;

class OauthApiController extends Controller {
/** @var AccessTokenMapper */
Expand All @@ -58,6 +61,10 @@ class OauthApiController extends Controller {
private $time;
/** @var Throttler */
private $throttler;
/** @var IUserSession */
private $userSession;
/** @var IUrlGenerator */
private $urlGenerator;

public function __construct(string $appName,
IRequest $request,
Expand All @@ -67,7 +74,9 @@ public function __construct(string $appName,
TokenProvider $tokenProvider,
ISecureRandom $secureRandom,
ITimeFactory $time,
Throttler $throttler) {
Throttler $throttler,
IUserSession $userSession,
IURLGenerator $urlGenerator) {
Comment on lines +77 to +79
Copy link
Contributor

@kesselb kesselb Mar 27, 2020

Choose a reason for hiding this comment

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

image

apps/oauth2/tests/Controller/OauthApiControllerTest.php needs an update for the new dependencies.

Copy link
Author

@jlallana jlallana Mar 27, 2020

Choose a reason for hiding this comment

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

Right, I fix it and resend the pull request

parent::__construct($appName, $request);
$this->crypto = $crypto;
$this->accessTokenMapper = $accessTokenMapper;
Expand All @@ -76,6 +85,8 @@ public function __construct(string $appName,
$this->secureRandom = $secureRandom;
$this->time = $time;
$this->throttler = $throttler;
$this->userSession = $userSession;
$this->urlGenerator = $urlGenerator;
}

/**
Expand Down Expand Up @@ -177,4 +188,35 @@ public function getToken($grant_type, $code, $refresh_token, $client_id, $client
]
);
}

/**
* @PublicPage
* @NoCSRFRequired
*
* @return JSONResponse
*/
public function discovery() {
$util = new Util();
Copy link
Contributor

@kesselb kesselb Mar 27, 2020

Choose a reason for hiding this comment

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

Not used.

Copy link
Author

@jlallana jlallana Mar 27, 2020

Choose a reason for hiding this comment

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

Probably

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
* @NoCSRFRequired
*
* @return JSONResponse
*/
public function getUserInfo() {
$user = $this->userSession->getUser();
return new JSONResponse([
'sub' => $user->getUID(),
'name' => $user->getDisplayName(),
'email' => $user->getEMailAddress()
]);
}
}