Use provider session and refresh tokens

[akhil1508]

trafficstars

• Attempt to resolve https://github.com/pulsejet/nextcloud-oidc-login/issues/102
• Keeping in draft as it is being tested
  • Testing with Nextcloud 24 and Keycloak
• This can also help with performing login at nextcloud webmail(rainloop or snappymail for instance) using the access token from the OIDC provider
• Also did a small refactor of moving the createOIDCClient function into a new service called TokenService

[akhil1508]

@stevesobol @pulsejet Please take a look whenever possible :)

[pulsejet]

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 BeforeTemplateRenderedEvent and not e.g. every request? Wouldn't this e.g. lead to different behavior between web and the mobile apps?

[akhil1508]

@pulsejet

One comment: why check refresh tokens in BeforeTemplateRenderedEvent and not e.g. every request?

• Hmm, every request sounds better tbh. So if we put it in the boot function of the app like the logic that is there already over here, it happens on every request? My main reasoning behind not keeping it in the boot and instead keeping it with a listener was that I saw the refreshTokens function being called even on requests for assets(Icons, CSS, JS).

Wouldn't this e.g. lead to different behavior between web and the mobile apps?

• Makes sense, API calls from mobile apps should also have the right session params set.

[akhil1508]

@pulsejet Wdyt about it now, I added the check in boot in Application.php

[shreyasajj]

Hi @azmeuk @sirkrypt0 Could you look at this, I would love to have this if possible

[azmeuk]

Hi. Reading the code it looks OK though I have not tested it yet. Can you help with testing @shreyasajj?

[shreyasajj]

Yea I don't mind but it might take a couple days to make sure it doesnt sign me out

[akhil1508]

Hi. Reading the code it looks OK though I have not tested it yet. Can you help with testing @shreyasajj?

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?

[shreyasajj]

I am getting this error Error: Undefined property: stdClass::$refresh_expires_in at /var/www/html/custom_apps/oidc_login/lib/Service/TokenService.php#102 and Error: Undefined property: stdClass::$refresh_token at /var/www/html/custom_apps/oidc_login/lib/Service/TokenService.php#98

[shreyasajj]

I am not sure how to test the WebDav, could you give me some guidance on this?

[akhil1508]

I am getting this error Error: Undefined property: stdClass::$refresh_expires_in at /var/www/html/custom_apps/oidc_login/lib/Service/TokenService.php#102 and Error: Undefined property: stdClass::$refresh_token at /var/www/html/custom_apps/oidc_login/lib/Service/TokenService.php#98

@shreyasajj Is refresh token functionality configured in your OIDC server?

I make it optional to enable refreshing

[akhil1508]

@shreyasajj

I am not sure how to test the WebDav, could you give me some guidance on this?

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 storeTokens is called only in the controller login method and refreshTokens is only called if is_oidc is true in the session and this is true only if the login happens through this app.

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.

[shreyasajj]

I am getting this error Error: Undefined property: stdClass::$refresh_expires_in at /var/www/html/custom_apps/oidc_login/lib/Service/TokenService.php#102 and Error: Undefined property: stdClass::$refresh_token at /var/www/html/custom_apps/oidc_login/lib/Service/TokenService.php#98

@shreyasajj Is refresh token functionality configured in your OIDC server?

I make it optional to enable refreshing

Yes I did, I have this line in authelia

grant_types:
          - refresh_token
          - authorization_code

based on https://www.authelia.com/configuration/identity-providers/open-id-connect/#grant_types

[akhil1508]

Yes I did, I have this line in authelia

@shreyasajj Do you confirm that a refresh token is returned in the response?

[Adphi]

Hi @akhil1508, thanks for your work on this topic 😀.

You don't need a configuration parameter to enable refresh tokens. Refresh tokens should be enabled if the requested scopes contain offline_access, see the request parameters definition and the scope definitions.

[Adphi]

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.

Undefined property: stdClass::$refresh_expires_in at /var/www/html/custom_apps/oidc_login/lib/Service/TokenService.php

Dex does not send the refresh_expires_in field as it does not exists in the official response format.

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);
     }
 }

[Adphi]

The logout does not work because its URL is built during login. Therefore, the token in id_token_hint is expired when the user is redirected to it.

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;
     }
 }

[akhil1508]

@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!

[Adphi]

@akhil1508 you're welcome !

I updated my previous comment to add the fix.

[akhil1508]

You don't need a configuration parameter to enable refresh tokens. Refresh tokens should be enabled if the requested scopes contain offline_access, see the request parameters definition and the scope definitions.

@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

[Adphi]

@akhil1508 thanks !

I think there are two different things:

• if the scope contains offline_access, the token needs to be refreshed to be able to logout (if a logout endpoint is defined in the openid discovery).
• Should the openid provider be used to provide the sessions ? I don't know why an administrator would ask for offline_access if not to handle user sessions. I agree that this should be an explicit setting, and defining offline_access in the scopes seems to me to be explicit enough.

What do you think @pulsejet ?

[akhil1508]

@pulsejet @Adphi Requesting you to look into this one :)

I got a little busy in between and was unable to push it forward earlier but can resolve issues promptly now.

[akhil1508]

I don't know why an administrator would ask for offline_access if not to handle user sessions. I agree that this should be an explicit setting, and defining offline_access in the scopes seems to me to be explicit enough.

@Adphi My only issue is that at times, the default settings of SSO systems(keycloak) have offline_access in the scopes. Some user might just want to login but maintain session in cloud itself. That was my idea of having a setting.

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 false). Might be I am wrong with my reasoning here :)

if the scope contains offline_access, the token needs to be refreshed to be able to logout (if a logout endpoint is defined in the openid discovery).

So basically in postLogout listener I try to refresh tokens first and then logout?

[Adphi]

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 false). Might be I am wrong with my reasoning here :)

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.

if the scope contains offline_access, the token needs to be refreshed to be able to logout (if a logout endpoint is defined in the openid discovery).

So basically in postLogout listener I try to refresh tokens first and then logout?

Yes, that is the way to go. You need a valid id_token_hint in order to logout.

[akhil1508]

@Adphi Makes sense. I will make changes accordingly and re-push.

[akhil1508]

@Adphi So here, we need a check to see if oidc_login_scope as set by the user contains offline_access and in that case refresh token usage has to happen?

In the spec, we also have

The use of Refresh Tokens is not exclusive to the offline_access use case. The Authorization Server MAY grant Refresh Tokens in other contexts that are beyond the scope of this specification.

So what I understand is that when offline_access is requested, usage of request tokens is compulsary.

[Adphi]

So what I understand is that when offline_access is requested, usage of request tokens is compulsary.

@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

[akhil1508]

@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.

[TheTimeWalker]

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.

[jmallach]

What's the status of this PR?