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

Changes for tctl sso test, tctl sso configure commands [SAML] #11508

Merged
merged 61 commits into from
May 4, 2022

Conversation

Tener
Copy link
Contributor

@Tener Tener commented Mar 28, 2022

These are necessary changes to support tctl sso test and (to a much smaller degree) tctl sso configure command.

RFD: tctl sso configure command: #9845
RFD: tctl sso test command: #9775

See: #9270 for original issue, which covers larger scope: SAML, OIDC and GitHub auth connectors. This PR is only touching on SAML, but the implementations for OIDC and GitHub should be parallel to this one.

Actual commands for SAML are implemented in: https://github.com/gravitational/teleport.e/pull/425

Webapps PR: gravitational/webapps#717

@Tener Tener marked this pull request as ready for review March 29, 2022 14:47
@github-actions github-actions bot added the audit-log Issues related to Teleports Audit Log label Mar 29, 2022
@github-actions github-actions bot requested review from atburke and rosstimothy March 29, 2022 14:48
@Tener Tener requested review from r0mant and smallinsky March 29, 2022 14:48
@Tener
Copy link
Contributor Author

Tener commented Mar 29, 2022

Dear reviewers, but especially @r0mant @smallinsky : please take a close look at the security angle, including the topics mentioned in the respective RFDs. Thanks!

@Tener Tener changed the title Changes for tctl sso test, tctl sso configure commands Changes for tctl sso test, tctl sso configure commands [SAML] Mar 31, 2022
@Tener Tener added the sso Used for single sign on related tasks. label Mar 31, 2022
Copy link
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

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

First pass, Left some comments.

@Tener Tener requested a review from smallinsky April 5, 2022 13:21
@Tener
Copy link
Contributor Author

Tener commented Apr 22, 2022

I left a few more minor suggestions, the implementation is looking good to me now! But I think this is still missing test coverage?

@r0mant thanks for the review, I'll add test coverage next.

Tener and others added 8 commits April 22, 2022 09:18
Co-authored-by: Roman Tkachenko <roman@goteleport.com>
- Rename auth.AssertionInfoWrapper to shorter auth.AssertionInfo.
- Add bool TestFlow to SSODiagnosticInfo
- Make SSODiagnosticInfo.Success a bool instead of string.
- Rename SAMLAttributesToRolesWarnings to more generic SSOWarnings
- Add godocs in several places.
- Avoid explicit call to trace.AddUserMessage() where possible.
@Tener
Copy link
Contributor Author

Tener commented Apr 25, 2022

@r0mant I've added test coverage for the critical pieces. Let me know if you'd like me to add more coverage anywhere.

@Tener Tener requested a review from r0mant April 26, 2022 14:57
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

@Tener Ship it! But after 2nd approval :)

@Tener
Copy link
Contributor Author

Tener commented Apr 28, 2022

@Tener Ship it! But after 2nd approval :)

Awesome, thanks for approval.

@smallinsky PTAL?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-log Issues related to Teleports Audit Log sso Used for single sign on related tasks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants