Implement token exchange mechanism for other apps #974

julien-nc · 2024-10-30T13:19:45Z

Some integration apps (like integration_openproject) might need to make backend-to-backend requests to other services that use the same IdP as Nextcloud. If those services accept Oidc access tokens as a Bearer tokens for authentication, they might only accept the ones that were delivered for them (for their audience).

Token exchange is a mechanism to exchange a token that was delivered for an audience (for a specific Oidc client) against another token for another audience.

The idea here is to keep the token exchange logic in user_oidc so it saves the effort to reimplement it in all the apps that need it.

In short:

The login token is stored in the Php session
It is refreshed when needed
If it can't be refreshed, we try to reauthenticate (which might be transparent for the user if the IdP session is still alive)
We listen to the ExchangedTokenRequestedEvent
We exchange the login token when receiving the event and place the result in the event

cc @individual-it @SagarGi

SagarGi · 2024-11-05T04:20:55Z

hi @julien-nc thanks for the implementation. i will checkout this PR.

SagarGi

Hi @julien-nc I manually checked the PR.
I have a question for this implementation.

As far as i have checked it you are implementing as, when ever we need an exchanged token we need to emit the event?
And if we do so then i think every time we require an exchanged token we always generate a new one?

SagarGi · 2024-11-06T10:58:15Z

lib/Service/TokenService.php

+				$bodyArray,
+				['provider_id' => $loginToken->getProviderId()],
+			);
+			return new Token($tokenData);


To create Object of Token the $tokenData must have the id_token key. But after a token is exchanged the response does not have id_token(as far as i have tried for OIDC provider keycloak) included leading to throw an error. And also do we even require to save id_token ? AFAIK it is only needed for authentication?

Hmmm in my tests the exchanged token has the same "shape" as the login token, it contains an id token and an access token. So it might depend on the client config on the IdP side. We can make it so the Token object does not require an id_token.

The id_token might be used for anything, not just the login. It contains user info indeed.

Can you try to change line 41 of lib/Model/Token.php to:
$this->idToken = $tokenData['id_token'] ?? null;

and line 54 to:
public function getIdToken(): ?string {

Hmmm in my tests the exchanged token has the same "shape" as the login token, it contains an id token and an access token. So it might depend on the client config on the IdP side. We can make it so the Token object does not require an id_token.

Sure, I think we do not require the id token to be stored for client to client communication for oidc based authentication.

and 32 to:
private ?string $idToken;

yes i did and its working!

julien-nc · 2024-11-06T11:29:16Z

As far as i have checked it you are implementing as, when ever we need an exchanged token we need to emit the event?
And if we do so then i think every time we require an exchanged token we always generate a new one?

Yes and yes

SagarGi · 2024-11-07T04:10:35Z

@julien-nc We need token for each request i guess does that mean we emit that event and get token every time we make request to targeted client. would not be it too much?
CC @individual-it

individual-it · 2024-11-07T04:17:08Z

@julien-nc We need token for each request i guess does that mean we emit that event and get token every time we make request to targeted client. would not be it too much? CC @individual-it

yes I would do it exactly like that. The integration app emits the event every time it needs the token, that way only the oidc app has to care about expiration time, renewals, caching of tokens etc.

DaphneMuller · 2024-11-21T14:45:48Z

@SagarGi @juliusknorr any chance this can be reviewed soon?

nabim777 · 2024-11-26T11:34:27Z

lib/Service/TokenService.php

+				['provider_id' => $loginToken->getProviderId()],
+			);
+			return new Token($tokenData);
+		} catch (\Exception|\Throwable $e) {


Currently, if the audience is invalid, the code throws an error:
Failed to exchange token, error in the exchange request.

It would be better if it throws a more specific error for an invalid audience, as this would make it clearer and more helpful, especially for displaying in the UI.

On API, when the audience is invalid, we get status code 400 and the response:

{ "error": "invalid_client", "error_description": "Audience not found" }

Good point. The TokenExchangeFailedException class now has 2 additional methods:

public function getError(): ?string; public function getErrorDescription(): ?string;

They both return a string only if the exchange request fails and returns a JSON object with the error and error_description keys.

Oh! that's great!

juliusknorr · 2024-11-26T21:12:12Z

lib/Service/TokenService.php

+	public function getToken(bool $refreshIfExpired = true): ?Token {
+		$sessionData = $this->session->get(self::SESSION_TOKEN_KEY);
+		if (!$sessionData) {
+			$this->logger->debug('[TokenService] getToken: no session data');


Should this be logged at a higher level?

Maybe it will flood the logs since this one is produced not only when there's a token exchange request but also when checking the login token in Application.php. If the current user is not authenticated via user_oidc, there will never be any session data.

All right then debug is also fine.

juliusknorr · 2024-11-26T21:13:29Z

lib/Service/TokenService.php

+			$this->logger->debug('[TokenService] checkLoginToken: user not logged in');
+			return;
+		}
+		if ($this->config->getUserValue($currentUser->getUID(), Application::APP_ID, 'had_token_once', '0') !== '1') {


I think I miss understanding on why we need this extra user value check, maybe you can elaborate on that?

If the user never had any login token before, it means it's not using user_oidc to authenticate so we know we can skip the login token check.

Do you mean that it is enough to just do nothing if there is no login token in the session?
We need to know if there is no login token available

because user_oidc was not used to log in (we can silently skip the check)

OR because the session has died/disappeared (then we need to reauthenticate)

Does that make sense?

Ah ok, makes sense then 👍

juliusknorr

Two minor comments, otherwise that code and general structure looks good.

Actually this is nice as it could simplify logic in https://github.com/nextmcloud/nmc_spica/blob/main/lib/Service/TokenService.php where a similar thing has been done

…eep it fresh Signed-off-by: Julien Veyssier <[email protected]>

Signed-off-by: Julien Veyssier <[email protected]>

…token exchange fails Signed-off-by: Julien Veyssier <[email protected]>

nabim777 · 2024-11-28T10:55:10Z

I have reviewed on behalf of @SagarGi . After checking manually the PR, it meets the expectations and requirements. LGTM 👍

julien-nc added enhancement New feature or request 3. to review labels Oct 30, 2024

julien-nc requested review from juliusknorr and edward-ly October 30, 2024 13:19

julien-nc force-pushed the enh/925/token-exchange-service branch 2 times, most recently from 25e3aba to d27cde4 Compare November 4, 2024 14:39

SagarGi reviewed Nov 6, 2024

View reviewed changes

nabim777 reviewed Nov 26, 2024

View reviewed changes

juliusknorr reviewed Nov 26, 2024

View reviewed changes

juliusknorr approved these changes Nov 26, 2024

View reviewed changes

julien-nc added 4 commits November 27, 2024 10:45

add token exchange event-based mechanism, store the login token and k…

04a29b3

…eep it fresh Signed-off-by: Julien Veyssier <[email protected]>

add docs for token exchange

d98230c

Signed-off-by: Julien Veyssier <[email protected]>

allow empty id_token in Token objects

e1bc9f4

Signed-off-by: Julien Veyssier <[email protected]>

fix reuse issues

d097d04

Signed-off-by: Julien Veyssier <[email protected]>

julien-nc force-pushed the enh/925/token-exchange-service branch from 572e928 to 803ff21 Compare November 27, 2024 10:13

julien-nc requested review from nabim777 and SagarGi November 27, 2024 10:27

try to provide more details in the TokenExchangeFailedException when …

fba2265

…token exchange fails Signed-off-by: Julien Veyssier <[email protected]>

julien-nc force-pushed the enh/925/token-exchange-service branch from 803ff21 to fba2265 Compare November 27, 2024 10:31

julien-nc merged commit d18772d into main Nov 28, 2024
43 of 45 checks passed

julien-nc deleted the enh/925/token-exchange-service branch November 28, 2024 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement token exchange mechanism for other apps #974

Implement token exchange mechanism for other apps #974

julien-nc commented Oct 30, 2024 •

edited

Loading

SagarGi commented Nov 5, 2024

SagarGi left a comment

SagarGi Nov 6, 2024 •

edited

Loading

julien-nc Nov 6, 2024

julien-nc Nov 6, 2024

julien-nc Nov 6, 2024

julien-nc Nov 6, 2024

SagarGi Nov 6, 2024 •

edited by julien-nc

Loading

julien-nc Nov 6, 2024

SagarGi Nov 6, 2024

julien-nc commented Nov 6, 2024

SagarGi commented Nov 7, 2024

individual-it commented Nov 7, 2024

DaphneMuller commented Nov 21, 2024

nabim777 Nov 26, 2024

julien-nc Nov 27, 2024

nabim777 Nov 28, 2024

juliusknorr Nov 26, 2024

julien-nc Nov 27, 2024

juliusknorr Nov 27, 2024

juliusknorr Nov 26, 2024

julien-nc Nov 27, 2024

juliusknorr Nov 27, 2024

juliusknorr left a comment

nabim777 commented Nov 28, 2024 •

edited

Loading

Implement token exchange mechanism for other apps #974

Implement token exchange mechanism for other apps #974

Conversation

julien-nc commented Oct 30, 2024 • edited Loading

SagarGi commented Nov 5, 2024

SagarGi left a comment

Choose a reason for hiding this comment

SagarGi Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SagarGi Nov 6, 2024 • edited by julien-nc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien-nc commented Nov 6, 2024

SagarGi commented Nov 7, 2024

individual-it commented Nov 7, 2024

DaphneMuller commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliusknorr left a comment

Choose a reason for hiding this comment

nabim777 commented Nov 28, 2024 • edited Loading

julien-nc commented Oct 30, 2024 •

edited

Loading

SagarGi Nov 6, 2024 •

edited

Loading

SagarGi Nov 6, 2024 •

edited by julien-nc

Loading

nabim777 commented Nov 28, 2024 •

edited

Loading