New commands: tctl sso test, tctl sso configure for GitHub#12783
Conversation
|
Kind reminder @Joerger @zmb3 @r0mant @xacrimon @smallinsky . |
smallinsky
left a comment
smallinsky
left a comment
There was a problem hiding this comment.
LGTM when remaining comments will be addressed.
| diagCtx.info.Error = trace.UserMessage(err) | ||
|
|
||
| diagCtx.writeToBackend(ctx) |
There was a problem hiding this comment.
Can we simplify this to:
diagCtx := a.newSSODiagContext(types.KindSAML)
defer diagCtx.writeToBackend(ctx)
auth, err := a.validateSAMLResponse(ctx, diagCtx, samlResponse)
if err != nil {
diagCtx.info.Error = trace.UserMessage(err)
}There was a problem hiding this comment.
Huh, I think the current code is simpler? We used to check against err, but trace.UserMessage handles that already, so the conditional is actually redundant.
diagCtx := m.newSSODiagContext(types.KindGithub)
auth, err := m.validateGithubAuthCallback(ctx, diagCtx, q)
diagCtx.info.Error = trace.UserMessage(err)
diagCtx.writeToBackend(ctx)Personally, I think defer makes it harder to reason about the function. Do we really need it?
|
@smallinsky thanks for review, hopefully I addressed your points; do let me know what you think about #12783 (comment) |
|
|
||
| // GetGithubAuthRequest gets Github AuthnRequest | ||
| func (c *Client) GetGithubAuthRequest(ctx context.Context, id string) (*services.GithubAuthRequest, error) { | ||
| out, err := c.Get(ctx, c.Endpoint("github", "requests", "get", id), url.Values{}) |
There was a problem hiding this comment.
Is there a reason that this needs to be an http request rather than gRPC? lib/auth.Client is deprecated in favor of api/client.Client, which is the purely gRPC client - #6394
There was a problem hiding this comment.
Adding the gRPC endpoint would require adding the relevant types to .proto files and more refactoring besides. This is not necessarily the wrong thing to do, but I'm afraid it would balloon the size of this already large PR even more. Also, I think it would make sense to move all methods from a given group in one go, rather than have some implemented on HTTP side, and some on gRPC side.
There was a problem hiding this comment.
I think that making new http endpoints only makes things more confusing for developers. IMO it'd be better to take a hard lined approach of only adding new endpoints to gRPC, even if that fits less smoothly in the current code base. Otherwise we may have more instances of http endpoints being added needlessly, moving us further away from having a purely gRPC, public API, in one place.
Before this gets pushed to a release branch, we should move these new (and old) http endpoints to gRPC. I can create a follow up PR for this.
There was a problem hiding this comment.
Before this gets pushed to a release branch, we should move these new (and old) http endpoints to gRPC. I can create a follow up PR for this.
Sure, let's do it. There should be enough time before the v10 release to make this happen. This feature won't be merged to v9 either.
4 participants
Follow up to:
tctl sso test,tctl sso configurecommands [SAML] #11508tctl sso test,tctl sso configurecommands [OIDC] #12519This PR adds
tclt sso configure githubandtctl sso testcommands. The commands are described in RFDs:tctl sso configure: https://github.com/gravitational/teleport/blob/master/rfd/0070-tctl-sso-configure-command.mdtctl sso testhttps://github.com/gravitational/teleport/blob/master/rfd/0071-tctl-sso-test-command.mdThis is the last code piece of work for #9270. (Some work remains before we can close the issue entirely.)
This PR requires follow up changes in
teleport.e, done under this PR: https://github.com/gravitational/teleport.e/pull/434