Skip to content
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.

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

Support differing expiry of identity vs authorization OAuth tokens #24815

Open
2 tasks done
acierto opened this issue May 17, 2024 · 9 comments
Open
2 tasks done

Support differing expiry of identity vs authorization OAuth tokens #24815

acierto opened this issue May 17, 2024 · 9 comments
Labels
auth enhancement New feature or request help wanted Help/Contributions wanted from community members

Comments

@acierto
Copy link
Contributor

acierto commented May 17, 2024

🔖 Need

Current issue:

If to open Backstage in 2 different tabs and perform ScmAuthApi.getCredentials in each tab, the token on tab 1 will be revoked. The problem lies that the session lives only in memory and the session manager doesn't reuse it.

It's not unique when users can open Backstage in multiple tabs or in different browsers, in order to perform different scenarios in each browser session.

🎉 Proposal

The proposal is to store the OAuth session in the database, for example in backstage_plugin_auth database in a new table oauth_session or something like this.

〽️ Alternatives

No response

❌ Risks

Such tokens are short lived, i.e. GitLab token can be valid maximum for 2 hours.

👀 Have you spent some time to check if this RFC has been raised before?

  • I checked and didn't find similar issue

🏢 Have you read the Code of Conduct?

@acierto acierto added rfc Request For Comment(s) auth labels May 17, 2024
@Rugvip
Copy link
Member

Rugvip commented May 17, 2024

Which provider is it that you're seeing this for? The token being revoked is something that shouldn't happen regardless of whether we have backend storage or not. Agree that that would be a good addition though 👍

There are some potential fixes for this on the way in #24743 too, I spotted some bugs for the refresh endpoint of some providers as part of that refactor.

@acierto
Copy link
Contributor Author

acierto commented May 17, 2024

I see it for GitLab provider.
I could reproduce it by opening 2 different scaffolder templates in separate tabs, where both require to get an access token

@dariozachow
Copy link

We are using the ID Token from GitLab to request some third party API and calling the gitlabAuthApi.getIdToken() everytime before the requests is made. The ID Token is valid just for 2mins thus every request after that time will fail. This makes it very annoying for the user as he must reload the page to get a new login window to get a fresh token. Background refreshing of the Token would be very helpful 🙏

@freben
Copy link
Member

freben commented Jun 24, 2024

It's not technically possible to do it in background. And besides, even if it were possible, as reported at the top, that might even more aggressively revoke other ongoing token flows causing problems elsewhere right?

It sounds to me like this is a serious limitation in how gitlab does token handling in general. Any pointers or assistance in making this better is appreciated 🙏

@floric
Copy link

floric commented Jun 25, 2024

I checked the responses from Gitlab. The ID token has an expiration time of only two minutes. But the expiration date returned in the response is 120 minutes. The access token is also 120 minutes valid, if not revoked by a refresh. How often does Backstage refresh the token? If I understood it correctly, it will be refreshed only 5 minutes before it's expiration. This would mean after 115 minutes. At this time the ID token will always be expired.

EDIT: What would help us?
a) to be able to configure the refresh action earlier manually
b) to be able to trigger a token refresh manually

@freben
Copy link
Member

freben commented Jun 27, 2024

It chooses an expiration that's capped by the access token expiry yes, to ensure that the refresh happens before it expires. So five minutes before that.

We don't look at the expiry time of the identity token as far as I can recall. Either way, two minutes is extremely short - I'm not entirely sure what's best to do here. Should we refresh it every minute in the background for example, whether it's used or not? That would be pretty aggressive and since each refresh kills any OTHER tokens that were issued for other purposes elsewhere, I worry that it would lead to problems in other places whose tokens would start to effectively become flaky.

At this point in time I don't think we have any great path forward for this. Potentially we could put code in place that ends up computing the refresh time to 1 minute 45 seconds or whatnot, instead of 120 seconds. That could be a contribution to the project if somebody is up for it, but I do worry about the implications of it. The even more complex path forward would be to make the code smart enough to only ask for refreshes if the specific token being asked for is near its expiry, i.e. making effectively two different expiry timestamps. Then those who never call getIdToken will see two-hour refreshes, and those who do use getIdToken see two-minute refreshes.

Removing the RFC state and making it a feature request instead.

@freben freben added enhancement New feature or request and removed rfc Request For Comment(s) labels Jun 27, 2024
@freben freben changed the title 💬 RFC: Make the usage of OAuth Access Token more stable/predictable Support differing expiry of identity vs authorization OAuth tokens Jun 27, 2024
@freben freben added the help wanted Help/Contributions wanted from community members label Jun 27, 2024
@floric
Copy link

floric commented Jun 28, 2024

Thanks for your research. We decided to use a Yarn patch as a hotfix like this:

// temporary fix for ID token expiration after 2 minutes, so refresh after 90 seconds instead of 120 minutes
if (provider.id === "gitlab") {
    return ((session.providerInfo.expiresAt.getTime() - Date.now()) / 1e3) < 7110;
}

Of course this is pretty hacky in apis/implementations/auth/oauth2/OAuth2.ts but solves the for us urgent issue for now. To be honest, I personally also think it should be fixed on the Gitlab side but there is an open issue for one year now. Maybe we can really consider this as a workaround hotfix, maybe with a feature flag?

@camilaibs
Copy link
Contributor

camilaibs commented Jul 8, 2024

It appears that there is an intention of having separate provider checks in the sessionShouldRefresh function as mentioned by @Rugvip in this comment:

// TODO(Rugvip): Optimize to use separate checks for provider vs backstage session expiration

I'm adding a needs-discussion label to hear from the team their recommendation.

@camilaibs camilaibs added needs discussion Bring up for discussion during next sync and removed needs discussion Bring up for discussion during next sync labels Jul 8, 2024
@camilaibs
Copy link
Contributor

Hi there, in conversation with @Rugvip, our initial recommendation would be adding a logic in the frontend that checks the expiration if the token is JWT token. Let us know if you have follow-up questions 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth enhancement New feature or request help wanted Help/Contributions wanted from community members
Projects
None yet
Development

No branches or pull requests

6 participants