Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

feat/sg: add 'sg sams client create'#63885

Merged
bobheadxi merged 5 commits into
mainfrom
07-17-feat_sg_add_sg_sams_client_create_
Jul 19, 2024
Merged

feat/sg: add 'sg sams client create'#63885
bobheadxi merged 5 commits into
mainfrom
07-17-feat_sg_add_sg_sams_client_create_

Conversation

@bobheadxi

@bobheadxi bobheadxi commented Jul 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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.

@cla-bot cla-bot Bot added the cla-signed label Jul 17, 2024
@bobheadxi Graphite App

bobheadxi commented Jul 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bobheadxi and the rest of your teammates on Graphite Graphite

@bobheadxi bobheadxi mentioned this pull request Jul 17, 2024
Merged
@bobheadxi bobheadxi force-pushed the 07-17-feat_sg_add_sg_sams_client_create_ branch from d550886 to 0e31f77 Compare July 17, 2024 18:39
@bobheadxi bobheadxi marked this pull request as ready for review July 17, 2024 18:41
@bobheadxi bobheadxi force-pushed the 07-17-feat_sg_add_sg_sams_client_create_ branch from 0e31f77 to e5397d4 Compare July 17, 2024 20:32
@bobheadxi bobheadxi force-pushed the sg-sams-default-credentials branch from d44553e to 9d9e2b9 Compare July 17, 2024 21:16
@bobheadxi bobheadxi force-pushed the 07-17-feat_sg_add_sg_sams_client_create_ branch from e5397d4 to cc5009b Compare July 17, 2024 21:16
eseliger
eseliger approved these changes Jul 19, 2024
View reviewed changes
Comment thread dev/sg/sams/sg_sams.go Outdated
Comment thread dev/sg/sams/sg_sams.go Outdated

@eseliger eseliger Jul 19, 2024

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 we also print localhost:9991 as an option for convenience so you don't have to research the port?

@bobheadxi bobheadxi Jul 19, 2024

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.

Added! Once Joe comes back I want to update the callback URLs of the various OAuth clients to point to a different port, so we can change the SAMS dev port to not conflict with embeddings

Comment thread dev/sg/sams/sg_sams.go Outdated

@eseliger eseliger Jul 19, 2024

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.

can we somehow not print this if sams URL is localhost so devs aren't that scared?

@bobheadxi bobheadxi Jul 19, 2024

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.

Added some stuff here to check the hostname of the target server, and if it's localhost or 127.0.0.1, we provide a gentler message about the client credentials only being shown once

@bobheadxi bobheadxi force-pushed the sg-sams-default-credentials branch from 9d9e2b9 to 0ec3c34 Compare July 19, 2024 15:43
@bobheadxi bobheadxi force-pushed the 07-17-feat_sg_add_sg_sams_client_create_ branch from 45c7236 to c285ce8 Compare July 19, 2024 15:44
@bobheadxi bobheadxi changed the base branch from sg-sams-default-credentials to graphite-base/63885 July 19, 2024 18:25
@bobheadxi bobheadxi changed the base branch from graphite-base/63885 to main July 19, 2024 18:25
@bobheadxi bobheadxi force-pushed the 07-17-feat_sg_add_sg_sams_client_create_ branch from 825404b to 0eda6d8 Compare July 19, 2024 18:25
@bobheadxi bobheadxi merged commit 30d50b7 into main Jul 19, 2024
@bobheadxi Graphite App

bobheadxi commented Jul 19, 2024

Copy link
Copy Markdown
Member Author

Merge activity

@bobheadxi bobheadxi deleted the 07-17-feat_sg_add_sg_sams_client_create_ branch July 19, 2024 20:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@eseliger eseliger eseliger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bobheadxi @eseliger