Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI ref for tctl sso commands. #13148

Merged
merged 17 commits into from
Jul 4, 2022
Merged

CLI ref for tctl sso commands. #13148

merged 17 commits into from
Jul 4, 2022

Conversation

Tener
Copy link
Contributor

@Tener Tener commented Jun 3, 2022

Taken out of another PR (#12941) to reduce scope.

Parent: #9270


Edit: There is now teams-to-roles field in the GitHub connector. The PR was updated to make the tctl sso configure github use it, instead of the legacy teams-to-logins.

Edit: Code changes moved to #13463.

docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved

Required params `--id` and `--secret` come from the [Github OAuth app](https://docs.github.com/en/developers/apps/building-oauth-apps/creating-an-oauth-app).

The flag `--teams-to-logins` can be provided multiple times to specify which Github Teams are assigned which roles.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we call this --teams-to-roles instead? teams-to-logins is a legacy name that predates RBAC, and there's actually an open PR that may make it into v10 which will rename the field in the connector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this is a good change, although I need to modify the actual code to make it so. This expands the scope of PR, but I'm happy to do so.

docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
@Tener Tener changed the title CLI ref for tctl sso commands. CLI ref for tctl sso commands. Use non-deprecated teams-to-roles field for GitHub. Jun 9, 2022
@Tener Tener requested a review from zmb3 June 9, 2022 14:11
@Tener
Copy link
Contributor Author

Tener commented Jun 9, 2022

@ptgott PTAL?

Copy link
Collaborator

@zmb3 zmb3 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 - all of my comments are just minor nits/grammar etc. Feel free to accept or ignore as many of them as you'd like.

docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Show resolved Hide resolved
Copy link
Collaborator

@zmb3 zmb3 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 - all of my comments are just minor nits/grammar etc. Feel free to accept or ignore as many of them as you'd like.

@Tener
Copy link
Contributor Author

Tener commented Jun 10, 2022

Looks good - all of my comments are just minor nits/grammar etc. Feel free to accept or ignore as many of them as you'd like.

These are good suggestions, thank you!

@Tener
Copy link
Contributor Author

Tener commented Jun 13, 2022

@ptgott can you please take a look?

docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved

The command supports all auth connector types: `github`, `oidc`, `saml`. The latter two require Teleport Enterprise.

The testing consists of running a single end-to-end authentication request using the provided auth connector definition.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put this paragraph after the command format? I think it would help readers to have the format in mind while reading this paragraph (e.g., the meaning of "the provided auth connector definition").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: 1d6841cfaf83f54e0092bba3c3150370695b2ccb


The testing consists of running a single end-to-end authentication request using the provided auth connector definition.
Once the request is finished, the results will be printed to console along with context-specific diagnostic information.
The test process may modify the list of configured auth connectors or result in new certificates being issued.
Copy link
Contributor

Choose a reason for hiding this comment

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

How can the test process modify the list of configured auth connectors? I think one or two brief examples would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this is actually a mistake: there should be negation there, as the test process will not do any of those things. I'll rephrase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: 1d6841cfaf83f54e0092bba3c3150370695b2ccb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ptgott PTAL

docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
@Tener Tener force-pushed the tener/tctl-sso-cli-ref branch from f26840f to 3ad8391 Compare June 14, 2022 09:07
@Tener Tener changed the title CLI ref for tctl sso commands. Use non-deprecated teams-to-roles field for GitHub. CLI ref for tctl sso commands. Jun 14, 2022
@Tener Tener requested a review from ptgott June 15, 2022 06:57
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@ptgott ptgott left a comment

Choose a reason for hiding this comment

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

Approved with some minor suggestions

docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
docs/pages/setup/reference/cli.mdx Outdated Show resolved Hide resolved
@Tener Tener enabled auto-merge (squash) July 4, 2022 14:52
@Tener Tener merged commit 2d75aac into master Jul 4, 2022
Tener added a commit that referenced this pull request Jul 4, 2022
* CLI ref for `tctl sso` commands.

Co-authored-by: Zac Bergquist <[email protected]>

Co-authored-by: Paul Gottschling <[email protected]>
Tener added a commit that referenced this pull request Jul 8, 2022
Co-authored-by: Zac Bergquist <[email protected]>

Co-authored-by: Paul Gottschling <[email protected]>
@webvictim webvictim mentioned this pull request Jul 12, 2022
@Tener Tener deleted the tener/tctl-sso-cli-ref branch July 22, 2022 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants