Skip to content

chore(hybrid-cloud): Delegate pipeline redis store to propagate customer domain#39016

Merged
dashed merged 1 commit intomasterfrom
hybrid-cloud/oauth2-customer-domains-pipeline
Sep 28, 2022
Merged

chore(hybrid-cloud): Delegate pipeline redis store to propagate customer domain#39016
dashed merged 1 commit intomasterfrom
hybrid-cloud/oauth2-customer-domains-pipeline

Conversation

@dashed
Copy link
Copy Markdown
Member

@dashed dashed commented Sep 19, 2022

In #38970, I had updated an OAuth2 provider implementation such that we do per-request customization by dynamically update the callback URL based on if a subdomain is present (e.g. sentry.sentry.io)

The OAuth2 spec on redirect URIs suggests that request-specific data in the callback URL be delegated to the "state" parameter.

I've undid the per-request callback URL changes added in #38970, and instead delegated to the pipeline redis store (using "state" parameter) to propagate any customer domain information.

The request flow for customer domains should look like this:

  1. Go to https://orgslug.sentry.io
  2. Click on social media button: https://sentry.io/identity/login/google/?referrer=login
  3. Create unique state parameter and store orgslug customer domain in pipeline redis store.
  4. Redirect to https://accounts.google.com/o/oauth2/auth
  5. Perform OAuth2 authorization flow.
  6. Redirect back to https://sentry.io/auth/sso/
  7. Authorization code token exchange.
  8. Retrieve orgslug customer domain from pipeline redis store, and redirect user to https://orgslug.sentry.io/auth/login
  9. Redirect to https://orgslug.sentry.o/organizations/orgslug/issues/ (this will be an orgless slug in the future)

@dashed dashed requested review from a team September 19, 2022 18:00
@dashed dashed requested a review from a team as a code owner September 19, 2022 18:00
@dashed dashed self-assigned this Sep 19, 2022
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 19, 2022
@dashed dashed requested a review from mdtro September 19, 2022 18:03
@dashed dashed force-pushed the hybrid-cloud/oauth2-customer-domains-pipeline branch from 87d7306 to c5690c8 Compare September 19, 2022 23:57
@dashed dashed force-pushed the hybrid-cloud/oauth2-customer-domains-pipeline branch from c5690c8 to 0ec004c Compare September 20, 2022 00:39
@dashed dashed force-pushed the hybrid-cloud/oauth2-customer-domains-pipeline branch from 0ec004c to fec27dd Compare September 27, 2022 00:25
Copy link
Copy Markdown
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Would be good to get someone from enterprise/ecosystem who is more familiar with our oauth implementation to also take a look


helper.bind_state("state", state)
if request.subdomain:
helper.bind_state("subdomain", request.subdomain)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should a user start and SSO flow, leave the tab open with oauth dance incomplete and then open a second tab, not complete that sso, and then go back and complete the first SSO flow they could end up at the wrong place.

In practice that is unlikely to matter as they should have access to both orgs that they started SSO on.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markstory Wouldn't the first SSO fail if the state attributed was regenerated on the second tab? We compare the state values here when the user comes back to Sentry from the authentication provider

if state != helper.fetch_state("state"):
return helper.error(ERR_INVALID_STATE)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree in practice it shouldn't matter.

Copy link
Copy Markdown
Contributor

@maxiuyuan maxiuyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, if ur cautious i would ask @RyanSkonnord to take a look as well

@dashed dashed merged commit f12d759 into master Sep 28, 2022
@dashed dashed deleted the hybrid-cloud/oauth2-customer-domains-pipeline branch September 28, 2022 17:53
ryan953 pushed a commit that referenced this pull request Oct 3, 2022
@evanpurkhiser
Copy link
Copy Markdown
Member

If you need more help looking at this in the future. I wrote all of the pipeline and oauth stuff

@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants