search

sourcegraph / sourcegraph-public-snapshot

Code AI platform with Code Search & Cody
https://sourcegraph.com
Other
10.1k stars 1.27k forks source link

Implement refresh token mechanism for all IDPs #36148

Open kopancek opened 2 years ago

kopancek commented 2 years ago

As part of incident https://sourcegraph.slack.com/archives/C03GQKP2NJF we have implemented a mechanism to refresh oauth tokens for gitlab identity provider. This is a ticking time bomb for the other identity providers as well as we did not implement refreshing tokens in any of the other providers. This issue was created to track work on longer term fix - implementing the refresh token mechanism for all the other providers and making sure the long-term solution is aligned across the code base.

We should take a look at how refreshing of an oauth token works: https://www.oauth.com/oauth2-servers/making-authenticated-requests/refreshing-an-access-token/

This would cover the following IDPs:

We need to check if anything needs to be done for SAML - See note below.

sourcegraph-bot-2 commented 2 years ago

Heads up @sourcegraph/iam-and-admin-experience - the "team/iam-admin-experience" label was applied to this issue.

unknwon commented 2 years ago

Related PRs that introduced the tech debt:

It is also worth noting that seems in the current fix for GitLab, we do not have any buffer time between the checkpoint and subsequent usages of the believed-to-be-valid token. For example, if the access token isn't expired at the point of the check, but we spend 30 seconds on listing repos from GitLab, it has a chance to expire.

RafLeszczynski commented 2 years ago

Hey team! Please add your planning poker estimate with ZenHub @kopancek @miveronese @pietrorosa77 @pjlast @unknwon

sourcegraph-bot-2 commented 2 years ago

Heads up @sourcegraph/iam - the "team/iam" label was applied to this issue.

miveronese commented 2 years ago

The refresh token mechanism (aka, "refreshing an expired token -> retrying the request") has been implemented/released in the following way so far:

My suggestion is to create separate issues for the remaining auth providers so we can keep track of what is missing and prioritize the next tasks accordingly. Will create them if it is ok for the team.

unknwon commented 2 years ago

My suggestion is to create separate issues for the remaining auth providers so we can keep track of what is missing and prioritize the next tasks accordingly. Will create them if it is ok for the team.

SGTM!

miveronese commented 2 years ago

Closing this one so we can track the support for the other auth provider via different issues.

miveronese commented 1 year ago

There's no need to refresh the token as the access token retrieved from the OIDC OP is only used in the session, during the sign-in/signout flow. This access token is not used in our codebase to fetch any resources/data that would require it to be refreshed in case it has expired.

When a user logs in with SAML, we don't store any type of token in the user_external_accout > auth_data column as we do when login happens via GH or GL. (Edited) But we store the SAML assertion response in the account_data column.

I tested different SAML configurations (having SG as a service provider and Auth0 as the Identity Provider) -- for example, apps with ID token expiration time and apps with also refresh token absolute expiration and inactive expiration -- , and no unexpected behavior was seen in the SG side.

It's important to note that SAML assertions can have an expiration date, but we are currently not checking this information when the user logs in and during the user session.

As far as I know, we don't offer OAuth for authentication with BitBucket. Users need to setup SAML for BB and login with Saml into SG.