This repository was archived by the owner on Sep 30, 2024. It is now read-only.
feat/sg: allow sg commands to default to local-dev SAMS-dev credentials#63883
Merged
Conversation
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @bobheadxi and the rest of your teammates on |
eseliger
approved these changes
Jul 17, 2024
d44553e to
9d9e2b9
Compare
9d9e2b9 to
0ec3c34
Compare
Member
Author
Merge activity
|
bobheadxi
referenced
this pull request
Jul 19, 2024
Adds an equivalent to the curl command we currently share, but in `sg`. If we add a better API around this later it's just an in-place replacement. Similar to https://github.com/sourcegraph/sourcegraph/pull/63883 this "just works" with zero configuration against SAMS-dev. Part https://linear.app/sourcegraph/issue/CORE-220, a spike into polishing some local-dev DX for SAMS. ## Test plan ``` sg sams client create -redirect-uris='https://sourcegraph.test:3443/.auth/callback' robert-testing ``` if you hit an error loading the secret, e.g. targeting the prod instance, you get a suggestion to get Entitle access: ``` sg sams client create -redirect-uris='https://sourcegraph.test:3443/.auth/callback' -sams='https://accounts.sourcegraph.com' robert-testing⚠️ Running sg with a dev build, following flags have different default value unless explictly set: skip-auto-update, disable-analytics 👉 Failed to get secret - do you have Entitle access to the "sourcegraph-accounts-prod-csvc" project? See https://sourcegraph.notion.site/Sourcegraph-Accounts-infrastructure-operations-b90a571da30443a8b1e7c31ade3594fb ❌ google(sourcegraph-accounts-prod-csvc): failed to get secret "MANAGEMENT_SECRET": rpc error: code = PermissionDenied desc = Permission 'secretmanager.versions.access' denied for resource 'projects/sourcegraph-accounts-prod-csvc/secrets/MANAGEMENT_SECRET/versions/latest' (or it may not exist). ``` ## Changelog - `sg sams client create` can now be used to create IdP clients for SAMS. --------- Co-authored-by: Erik Seliger <erikseliger@me.com>
unknwon
reviewed
Aug 14, 2024
Comment on lines
+55
to
+62
| if cfg.ClientID == "" { | ||
| cfg.ClientID, err = defaultLocalDevClientID(c.Context) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "get local dev client ID for %s", SAMSDevURL) | ||
| } | ||
| } | ||
| if cfg.ClientSecret == "" { | ||
| cfg.ClientSecret, err = defaultLocalDevClientSecret(c.Context) |
Contributor
There was a problem hiding this comment.
Given no possibility that a client secret can never work for more than one client ID, technically it is more clear to do
if cfg.ClientID == "" && cfg.ClientSecret == "" {
cfg.ClientID, err = defaultLocalDevClientID(c.Context)
...
cfg.ClientSecret, err = defaultLocalDevClientSecret(c.Context)
...
}otherwise only setting the client ID locally would use the default client secret, that "guaranteed" not gonna work?
Member
Author
There was a problem hiding this comment.
let's revisit this when we get the chance to revisit SAMS local DX as a whole! I'm not super happy with how tedious it is in general
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.
3 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.
As it says on the tin - various commands related to SAMS can now target dev services integrated against SAMS-dev directly. See test plan for examples.
I've also refactored the
sg sams introspect-tokenetc commands in preparation for introducing moresg samscommands - the existing commands are now collapsed intosg sams token introspectandsg sams token introspect -pPart https://linear.app/sourcegraph/issue/CORE-220, a spike into polishing some local-dev DX for SAMS.
I also upgrade the glamour library because I noticed the JSON pretty-printing was no longer colored - the upgrade fixed that
Test plan
All the below now work with no additional effort:
You can use it against locally running services that connect to SAMS-dev as well, for example the below also works with no additional flags/envvars:
sg start dotcom # includes enterprise-portal sg enterprise subscription list -enterprise-portal-server=http://localhost:6081Changelog
sgcommands requiring SAMS client credentials now load shared SAMS-dev client credentials by default.