Github connector #1512
Github connector #1512
Conversation
constants.go
Outdated
| SAML = "saml" | ||
|
|
||
| // Github means authentication will happen remotely using a Github connector. | ||
| Github = "github" |
There was a problem hiding this comment.
why not Github = ConnectorGithub
lib/auth/github.go
Outdated
|
|
||
| // GithubAuthResponse represents Github auth callback validation response | ||
| type GithubAuthResponse struct { | ||
| // Username is the name of authorized user |
lib/auth/github.go
Outdated
| if err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
| // exchange the authorization code we got in the callback for access token |
There was a problem hiding this comment.
instead of using we everywhere, can you please refer tot he code path, e.g. `exchange authorization code auth service got in the callback" here and below.
lib/auth/github.go
Outdated
| if err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
| if len(connector.GetTeamsToLogins()) != 0 { |
There was a problem hiding this comment.
should return error and log to the logs with Warning if there is not GetTeamsToLogins to simplify troubleshooting.
lib/auth/github.go
Outdated
| Kind: services.KindUser, | ||
| Version: services.V2, | ||
| Metadata: services.Metadata{ | ||
| Name: claims.Email, |
There was a problem hiding this comment.
why not github username and email? github username should be unique and not every user has email set?
There was a problem hiding this comment.
I am pretty sure email is mandatory for every Github account. We can use a username instead I think, should not be an issue.
There was a problem hiding this comment.
Only in this case we'd have to request the scope user:read which is a bit broader than user:email which we request now (and which grants permissions to read only email addresses).
There was a problem hiding this comment.
I've updated the code to use Github username instead of email. Actually username is public information so we don't need any additional scopes for it, so we only request read:org now.
lib/web/apiserver.go
Outdated
| logger := log.WithFields(log.Fields{trace.Component: "github"}) | ||
| logger.Debug("Web login start") | ||
| clientRedirectURL := r.URL.Query().Get("redirect_url") | ||
| if clientRedirectURL == "" { |
There was a problem hiding this comment.
was there a problem with random redirect_url that we had to check by stripping off the domain part and using relative vs absolute redirects?
There was a problem hiding this comment.
Yes, it is handled in the callback:
https://github.com/gravitational/teleport/blob/roman/github/lib/web/apiserver.go#L750
lib/web/apiserver.go
Outdated
| ConnectorID: req.ConnectorID, | ||
| PublicKey: req.PublicKey, | ||
| CertTTL: req.CertTTL, | ||
| CheckUser: true, |
There was a problem hiding this comment.
why check user is true here?
There was a problem hiding this comment.
I've originally implemented it similar to OIDC but then realized that this parameter was always true anyway, so I've just removed it now.
lib/web/apiserver.go
Outdated
| logger.Warnf("Unable to verify CSRF token: %v", err) | ||
| return nil, trace.AccessDenied("access denied") | ||
| } | ||
| logger.Infof("Callback redirecting to web browser") |
There was a problem hiding this comment.
missing dots at the end of log lines
lib/web/apiserver.go
Outdated
| httplib.SafeRedirect(w, r, response.Req.ClientRedirectURL) | ||
| return nil, nil | ||
| } | ||
| logger.Infof("Callback redirecting to console login") |
There was a problem hiding this comment.
Callback is redirecting
| @@ -16,6 +16,23 @@ limitations under the License. | |||
|
|
|||
| package ui | |||
|
|
|||
| const ( | |||
| // WebConfigAuthProviderOIDCType is OIDC provider type | |||
| WebConfigAuthProviderOIDCType = "oidc" | |||
| @@ -197,6 +197,11 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*RewritingHandler, error) { | |||
| h.GET("/webapi/saml/sso", httplib.MakeHandler(h.samlSSO)) | |||
| h.POST("/webapi/saml/login/console", httplib.MakeHandler(h.samlSSOConsole)) | |||
|
|
|||
| // Github connector handlers | |||
| h.GET("/webapi/github/login/web", httplib.MakeHandler(h.githubLoginWeb)) | |||
There was a problem hiding this comment.
if we have time, i suggest to use 1 endpoint for handling all types of oauth. web logins.
/webapi/oauth?type=XXX&rest...
It should remove duplicate code (which duplicates csrf checks) and make it easier to add a new type in the future (no need to modify web app code).
This is going to be a safe change as it's used only by web app.
There was a problem hiding this comment.
we don't have any other oauth logins at the moment, so this is not relevant.
There was a problem hiding this comment.
h.GET("/webapi/oidc/login/web", httplib.MakeHandler(h.oidcLoginWeb))
h.GET("/webapi/saml/sso", httplib.MakeHandler(h.samlSSO))
h.GET("/webapi/github/login/web", httplib.MakeHandler(h.githubLoginWeb))
all of them do the same thing for the most part. my suggestion is to use 1 endpoint and pass a provider type
There was a problem hiding this comment.
@alex-kovoy from Golang's URL router's perspective there's no difference between /one/two and /one?arg=two. you can route all 3 to the same function.
There was a problem hiding this comment.
@kontsevoy this is about code duplication. We have 3 functions doing the same thing for each above route. The suggestion is to have 1 function to reduce code duplication.
We do not need to have 3 different routes either.
There was a problem hiding this comment.
there are similar, but not the same, so let's keep it as is for now.
| @@ -534,10 +577,26 @@ func (h *Handler) getWebConfig(w http.ResponseWriter, r *http.Request, p httprou | |||
| // get all SAML connectors | |||
| samlConnectors, err := h.cfg.ProxyClient.GetSAMLConnectors(false) | |||
| if err != nil { | |||
| log.Infof("Cannot retrieve SAML connectors: %v", err) | |||
| log.Errorf("Cannot retrieve SAML connectors: %v.", err) | |||
There was a problem hiding this comment.
will it log an error if SAML connectors are not configured? (in which case this is not an error)
There was a problem hiding this comment.
Nope, it's not an error.
|
LGTM. I will move the docs to 2.4 manually later. |
Add support for Github connector. Closes #1445.