This repository was archived by the owner on Sep 30, 2024. It is now read-only.
fix/client/dev: update proxy overwrite to respect authProviders#63624
Merged
Conversation
Contributor
|
Are you saying you have to manually update the URL to match the original domain? Is there any way to make this work by default? |
Member
Author
|
@fkling I thought about that but I don't think it's possible, by design - it's generally hardcoded when you create an integration in the provider side, see my updated PR description. We also have to use a similar workaround to log in to customer Cloud instances. We could special-case it in SAMS to allow a configurable redirect URL provided by the client but I'm not sure that's secure, and nothing in the OAuth2 spec seems to accommodate something like that Note that you generally only have to do this once, until your session expires, which IIRC is 30 days |
…oxy-against-dotcom
fkling
approved these changes
Jul 9, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes https://linear.app/sourcegraph/issue/CORE-213
The existing overwrite stripping
window.context.authProviderssourced from a proxied API down to only built-in providers makes it impossible to reliably runsg start web-standaloneagainst Sourcegraph.com (and likely other Sourcegraph instances). The docstring says:However, you can log in with other OAuth2 providers locally, so it doesn't make sense to remove them from the context. This PR removes the overwrite so that we can log in to Sourcegraph.com with a locally running web app, and talk to Cody and all that good stuff.
How to log in to OAuth providers with
sg start web-standaloneIIRC what happens:
This redirect will surface an error, like so:
This happens because you get redirected to an absolute URL, which will be
sourcegraph.com(the upstream Sourcegraph instance), which won't match the cookie you get issued (sourcegraph.test). We just need to make sure it matches and send the code to the callback through our local proxy:And everything will work :) I confirmed this works for e.g. logging into S2 with GitHub as well, using the callback issued by GitHub.
We cannot make this the default behaviour, because the redirect URL is usually configured in the OAuth provider. An example GitHub OAuth application configuration:
Note their guidance on valid values: https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps#redirect-urls - we must provide a single, absolute URL
This is very similar to the process used to log in to private Cloud instances, e.g. https://cloud-ops.sgdev.org/dashboard/environments/prod/instances/src-bd02273f6b90d1d1beee#log-in-to-the-instance-ui (scroll to bottom of this section), which indicates we've thought about similar cases before and found that this was the best way
Test plan
https://www.loom.com/share/6cb3b3ca475b4b9392aa4b11938e76e6?sid=d99fcb59-4308-45c7-9f68-af9ac44c4e7e