Use provider session and refresh tokens #208
Conversation
akhil1508
changed the title
|
@stevesobol @pulsejet Please take a look whenever possible :) |
|
Overall this looks okay, but I forgot the details of how refresh tokens work so I need to revisit this later. One comment: why check refresh tokens in |
|
|
@pulsejet Wdyt about it now, I added the check in |
akhil1508
changed the title
|
Hi @azmeuk @sirkrypt0 Could you look at this, I would love to have this if possible |
|
Hi. Reading the code it looks OK though I have not tested it yet. Can you help with testing @shreyasajj? |
|
Yea I don't mind but it might take a couple days to make sure it doesnt sign me out |
Hello! Do let me know if you need any help from my side for testing. Important point to keep in mind is that oauth from dav clients also works correctly. As token is sent by the dav client(refresh should be done client-side in this case I believe), maybe we shouldn't try to update the token in that case wdyt? |
|
I am getting this error |
|
I am not sure how to test the WebDav, could you give me some guidance on this? |
@shreyasajj Is refresh token functionality configured in your OIDC server? I make it optional to enable refreshing |
Actually we don't need to test either basic or bearer auth backend for DAV as those are not affected at all by these changes. The function For DAV, I expect the clients to perform login against SSO server on their own and not via the app and then pass the token to validate if valid login so changes here should be fine. |
Yes I did, I have this line in authelia based on https://www.authelia.com/configuration/identity-providers/open-id-connect/#grant_types |
@shreyasajj Do you confirm that a refresh token is returned in the response? |
|
Hi @akhil1508, thanks for your work on this topic 😀. You don't need a configuration parameter to enable refresh tokens. |
|
I am testing the refresh implementation with dex and found a few problems. First, the implementation has zero logging, which makes debugging very complicated. Similarly, exceptions should not be silently ignored, they should at least be logged.
Dex does not send the I don't think the validity of the refresh token needs to be checked. If it is no longer valid, the refresh request fails and the user is logged out anyway. I finally got it working with the following patch: diff --git a/lib/Service/TokenService.php b/lib/Service/TokenService.php
index cf20bb9..22ca4bf 100644
--- a/lib/Service/TokenService.php
+++ b/lib/Service/TokenService.php
@@ -7,6 +7,7 @@ use OCA\OIDCLogin\Provider\OpenIDConnectClient;
use OCP\IConfig;
use OCP\ISession;
use OCP\IURLGenerator;
+use Psr\Log\LoggerInterface;
class TokenService
{
@@ -22,16 +23,21 @@ class TokenService
/** @var IURLGenerator */
private $urlGenerator;
+ private LoggerInterface $logger;
+
public function __construct(
$appName,
ISession $session,
IConfig $config,
IURLGenerator $urlGenerator,
- ) {
+ LoggerInterface $logger,
+ )
+ {
$this->appName = $appName;
$this->session = $session;
$this->config = $config;
$this->urlGenerator = $urlGenerator;
+ $this->logger = $logger;
}
/**
@@ -64,30 +70,34 @@ class TokenService
*/
public function refreshTokens(): bool
{
- $accessTokenExpiresIn = $this->session->get('oidc_access_token_expires_in');
+ $accessTokenExpiresAt = $this->session->get('oidc_access_token_expires_at');
$now = time();
+ $this->logger->debug("checking if token should be refreshed", ["expires" => $accessTokenExpiresAt]);
// If access token hasn't expired yet
- if (!empty($accessTokenExpiresIn) && $now < $accessTokenExpiresIn) {
+ if (!empty($accessTokenExpiresAt) && $now < $accessTokenExpiresAt) {
+ $this->logger->debug("no token expiration or not yet expired");
return true;
}
- $refreshTokenExpiresIn = $this->session->get('oidc_refresh_token_expires_in');
$refreshToken = $this->session->get('oidc_refresh_token');
// If refresh token doesn't exist or refresh token has expired
- if (empty($refreshToken) || (!empty($refreshTokenExpiresIn) && $now > $refreshTokenExpiresIn)) {
+ if (empty($refreshToken)) {
+ $this->logger->debug("refresh token not found");
return false;
}
- $callbackUrl = $this->urlGenerator->linkToRouteAbsolute($this->appName.'.login.oidc');
+ $callbackUrl = $this->urlGenerator->linkToRouteAbsolute($this->appName . '.login.oidc');
// Refresh the tokens, return false on failure
+ $this->logger->debug("refreshing token");
try {
$oidc = $this->createOIDCClient($callbackUrl);
$tokenResponse = $oidc->refreshToken($refreshToken);
$this->storeTokens($tokenResponse);
-
+ $this->logger->debug("token refreshed");
return true;
} catch (Exception $e) {
+ $this->logger->error("token refresh failed", ['exception' => $e]);
return false;
}
}
@@ -98,10 +108,8 @@ class TokenService
$this->session->set('oidc_refresh_token', $tokenResponse->refresh_token);
$now = time();
- $accessTokenExpiresIn = $tokenResponse->expires_in + $now;
- $refreshTokenExpiresIn = $now + $tokenResponse->refresh_expires_in - 5;
+ $accessTokenExpiresAt = $tokenResponse->expires_in + $now;
- $this->session->set('oidc_access_token_expires_in', $accessTokenExpiresIn);
- $this->session->set('oidc_refresh_token_expires_in', $refreshTokenExpiresIn);
+ $this->session->set('oidc_access_token_expires_at', $accessTokenExpiresAt);
}
}
|
|
The logout does not work because its URL is built during login. Therefore, the token in Final patch: diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index a0737c4..03a7bad 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -90,7 +90,7 @@ class Application extends App implements IBootstrap
$altLoginPage = $this->config->getSystemValue('oidc_login_alt_login_page', false);
// URL for login without redirecting forcefully, false if we are not doing that
- $noRedirLoginUrl = $useLoginRedirect ? $this->url->linkToRouteAbsolute('core.login.showLoginForm').'?noredir=1' : false;
+ $noRedirLoginUrl = $useLoginRedirect ? $this->url->linkToRouteAbsolute('core.login.showLoginForm') . '?noredir=1' : false;
// Get logged in user's session
$userSession = $container->query(IUserSession::class);
@@ -106,28 +106,31 @@ class Application extends App implements IBootstrap
// Disable password confirmation for user
$session->set('last-password-confirm', $container->query(ITimeFactory::class)->getTime());
+ $refreshTokensEnabled = $this->config->getSystemValue('oidc_refresh_tokens_enabled', false);
+
/* Redirect to logout URL on completing logout
If do not have logout URL, go to noredir on logout */
if ($logoutUrl = $session->get('oidc_logout_url', $noRedirLoginUrl)) {
- $userSession->listen('\OC\User', 'postLogout', function () use ($logoutUrl, $session) {
+ $userSession->listen('\OC\User', 'postLogout', function () use ($logoutUrl, $session, $refreshTokensEnabled) {
// Do nothing if this is a CORS request
if ($this->getContainer()->query(ControllerMethodReflector::class)->hasAnnotation('CORS')) {
return;
}
-
+ if ($refreshTokensEnabled) {
+ $logoutUrl = $this->tokenService->getLogoutUrl();
+ }
// Properly close the session and clear the browsers storage data before
// redirecting to the logout url.
$session->set('clearingExecutionContexts', '1');
$session->close();
header('Clear-Site-Data: "cache", "storage"');
- header('Location: '.$logoutUrl);
+ header('Location: ' . $logoutUrl);
exit;
});
}
- $refreshTokensEnabled = $this->config->getSystemValue('oidc_refresh_tokens_enabled', false);
if ($refreshTokensEnabled && !$this->tokenService->refreshTokens()) {
$userSession->logout();
}
@@ -160,7 +163,7 @@ class Application extends App implements IBootstrap
// Force redirect
if ($useLoginRedirect) {
- header('Location: '.$loginLink);
+ header('Location: ' . $loginLink);
exit;
}
diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php
index 8459aa5..0e7aeb6 100644
--- a/lib/Controller/LoginController.php
+++ b/lib/Controller/LoginController.php
@@ -94,6 +94,7 @@ class LoginController extends Controller
$oidc->authenticate();
$tokenResponse = $oidc->getTokenResponse();
+
$refreshTokensEnabled = $this->config->getSystemValue('oidc_refresh_tokens_enabled', false);
if ($refreshTokensEnabled) {
diff --git a/lib/Service/TokenService.php b/lib/Service/TokenService.php
index cf20bb9..ed16fd6 100644
--- a/lib/Service/TokenService.php
+++ b/lib/Service/TokenService.php
@@ -7,6 +7,7 @@ use OCA\OIDCLogin\Provider\OpenIDConnectClient;
use OCP\IConfig;
use OCP\ISession;
use OCP\IURLGenerator;
+use Psr\Log\LoggerInterface;
class TokenService
{
@@ -22,16 +23,23 @@ class TokenService
/** @var IURLGenerator */
private $urlGenerator;
+ private LoggerInterface $logger;
+
+ private string $logout_url = '';
+
public function __construct(
$appName,
ISession $session,
IConfig $config,
IURLGenerator $urlGenerator,
- ) {
+ LoggerInterface $logger,
+ )
+ {
$this->appName = $appName;
$this->session = $session;
$this->config = $config;
$this->urlGenerator = $urlGenerator;
+ $this->logger = $logger;
}
/**
@@ -64,44 +72,61 @@ class TokenService
*/
public function refreshTokens(): bool
{
- $accessTokenExpiresIn = $this->session->get('oidc_access_token_expires_in');
+ $accessTokenExpiresAt = $this->session->get('oidc_access_token_expires_at');
$now = time();
+ $this->logger->debug("checking if token should be refreshed", ["expires" => $accessTokenExpiresAt]);
// If access token hasn't expired yet
- if (!empty($accessTokenExpiresIn) && $now < $accessTokenExpiresIn) {
+ if (!empty($accessTokenExpiresAt) && $now < $accessTokenExpiresAt) {
+ $this->logger->debug("no token expiration or not yet expired");
return true;
}
- $refreshTokenExpiresIn = $this->session->get('oidc_refresh_token_expires_in');
$refreshToken = $this->session->get('oidc_refresh_token');
// If refresh token doesn't exist or refresh token has expired
- if (empty($refreshToken) || (!empty($refreshTokenExpiresIn) && $now > $refreshTokenExpiresIn)) {
+ if (empty($refreshToken)) {
+ $this->logger->debug("refresh token not found");
return false;
}
- $callbackUrl = $this->urlGenerator->linkToRouteAbsolute($this->appName.'.login.oidc');
+ $callbackUrl = $this->urlGenerator->linkToRouteAbsolute($this->appName . '.login.oidc');
// Refresh the tokens, return false on failure
+ $this->logger->debug("refreshing token");
try {
$oidc = $this->createOIDCClient($callbackUrl);
$tokenResponse = $oidc->refreshToken($refreshToken);
$this->storeTokens($tokenResponse);
+ if ($this->session->get('oidc_logout_url')) {
+ $this->logger->debug("updating logout url");
+ $oidc_login_logout_url = $this->config->getSystemValue('oidc_login_logout_url', false);
+ $this->logout_url = $oidc->getEndSessionUrl($oidc_login_logout_url);
+ $this->session->set('oidc_logout_url', $this->logout_url);
+ }
+ $this->logger->debug("token refreshed");
return true;
} catch (Exception $e) {
+ $this->logger->error("token refresh failed", ['exception' => $e]);
return false;
}
}
public function storeTokens(object $tokenResponse): void
{
+ $oldAccessToken = $this->session->get('oidc_access_token');
+ $this->logger->debug("oldAccessToken: " . $oldAccessToken);
+ $this->logger->debug("newAccessToken: " . $tokenResponse->access_token);
$this->session->set('oidc_access_token', $tokenResponse->access_token);
$this->session->set('oidc_refresh_token', $tokenResponse->refresh_token);
$now = time();
- $accessTokenExpiresIn = $tokenResponse->expires_in + $now;
- $refreshTokenExpiresIn = $now + $tokenResponse->refresh_expires_in - 5;
+ $accessTokenExpiresAt = $tokenResponse->expires_in + $now;
- $this->session->set('oidc_access_token_expires_in', $accessTokenExpiresIn);
- $this->session->set('oidc_refresh_token_expires_in', $refreshTokenExpiresIn);
+ $this->session->set('oidc_access_token_expires_at', $accessTokenExpiresAt);
+ }
+
+ public function getLogoutUrl()
+ {
+ return $this->logout_url;
}
}
|
|
@Adphi Thanks for the tests and comments! Yes, I shall add debug logs and also handle exceptions correctly I also apply fixes according to your comment(s) and push asap. Again, thanks! |
|
@akhil1508 you're welcome ! I updated my previous comment to add the fix. |
Signed-off-by: Akhil <[email protected]>
@Adphi I added a config parameter to leave the choice to the user to continue using the nextcloud session or instead check validity of session against the oidc server using tokens. Do you think this should not be a config parameter and should instead be default if the scope contains offline_access? Also pushed after making your changes, please take a look |
|
@akhil1508 thanks ! I think there are two different things:
What do you think @pulsejet ? |
Signed-off-by: Akhil <[email protected]>
@Adphi My only issue is that at times, the default settings of SSO systems(keycloak) have An admin who knows what they are doing v/s a selfhoster who just sets things up(catering to this as the default with the setting set to
So basically in |
I really think the defaults should respect the rfc, if keycloak doesn't, it shouldn't be an implementation problem for the client. You can implement a way to force not to refresh the token or not to delagate the session to the OP, but, still, the defaults should respect the rfc.
Yes, that is the way to go. You need a valid id_token_hint in order to logout. |
|
@Adphi Makes sense. I will make changes accordingly and re-push. |
|
@Adphi So here, we need a check to see if In the spec, we also have
So what I understand is that when |
@akhil1508 I understand it in the same way as you do. Which is perfectly in line with the idea set out above: > Refresh tokens should be enabled if the requested scopes contain offline_access |
Signed-off-by: Akhil <[email protected]>
|
@Adphi Done, applied your suggestions. Please check if the code looks good to you. In the meantime, I am testing thoroughly on my test installation. |
Signed-off-by: Akhil <[email protected]>
|
Hey there! Thanks for the PR. I have used that one on my own to use access and refresh tokens as well. However, I have wondered if there is even an advantage to using it as the refresh token in this PR? Right now the problem is that it looks like the session lifetime is still dictated by Nextcloud. As far as I understood, the idea of the PR would be to actually override this and only logout if the access token cannot be refreshed anymore. I personally can see that a logout is enforced if it can't refresh, but I can't see in the code that it would keep the session alive on Nextcloud-side if that's still valid. I would be very glad if you could explain if it's keeping the session alive on your side and if there is anything special to keep in mind for configuring on Nextcloud-side. |
|
What's the status of this PR? |
createOIDCClientfunction into a new service calledTokenService