From d2ffef2c7e0a622295c42cb2fdd70ef14593d03c Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Tue, 1 Dec 2020 17:50:27 -0800 Subject: [PATCH] Use global OIDC fields for Gitlab --- pkg/validation/options.go | 1 - providers/gitlab.go | 25 +++++++++++-------------- providers/oidc.go | 29 +++++++++++++++-------------- providers/provider_data.go | 12 ++++++------ providers/util.go | 16 ++++++++-------- 5 files changed, 40 insertions(+), 43 deletions(-) diff --git a/pkg/validation/options.go b/pkg/validation/options.go index d5e58312..4fc0b0a4 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -287,7 +287,6 @@ func parseProviderInfo(o *options.Options, msgs []string) []string { msgs = append(msgs, "oidc provider requires an oidc issuer URL") } case *providers.GitLabProvider: - p.AllowUnverifiedEmail = o.InsecureOIDCAllowUnverifiedEmail p.Groups = o.GitLabGroup err := p.AddProjects(o.GitlabProjects) if err != nil { diff --git a/providers/gitlab.go b/providers/gitlab.go index 246dd78c..c5922abb 100644 --- a/providers/gitlab.go +++ b/providers/gitlab.go @@ -20,8 +20,6 @@ type GitLabProvider struct { Groups []string Projects []*GitlabProject - - AllowUnverifiedEmail bool } // GitlabProject represents a Gitlab project constraint entity @@ -103,7 +101,7 @@ func (p *GitLabProvider) Redeem(ctx context.Context, redirectURL, code string) ( if err != nil { return nil, fmt.Errorf("token exchange: %v", err) } - s, err = p.createSessionState(ctx, token) + s, err = p.createSession(ctx, token) if err != nil { return nil, fmt.Errorf("unable to update session: %v", err) } @@ -162,7 +160,7 @@ func (p *GitLabProvider) redeemRefreshToken(ctx context.Context, s *sessions.Ses if err != nil { return fmt.Errorf("failed to get token: %v", err) } - newSession, err := p.createSessionState(ctx, token) + newSession, err := p.createSession(ctx, token) if err != nil { return fmt.Errorf("unable to update session: %v", err) } @@ -255,22 +253,21 @@ func (p *GitLabProvider) AddProjects(projects []string) error { return nil } -func (p *GitLabProvider) createSessionState(ctx context.Context, token *oauth2.Token) (*sessions.SessionState, error) { - rawIDToken, ok := token.Extra("id_token").(string) - if !ok { - return nil, fmt.Errorf("token response did not contain an id_token") - } - - // Parse and verify ID Token payload. - idToken, err := p.Verifier.Verify(ctx, rawIDToken) +func (p *GitLabProvider) createSession(ctx context.Context, token *oauth2.Token) (*sessions.SessionState, error) { + idToken, err := p.verifyIDToken(ctx, token) if err != nil { - return nil, fmt.Errorf("could not verify id_token: %v", err) + switch err { + case ErrMissingIDToken: + return nil, fmt.Errorf("token response did not contain an id_token") + default: + return nil, fmt.Errorf("could not verify id_token: %v", err) + } } created := time.Now() return &sessions.SessionState{ AccessToken: token.AccessToken, - IDToken: rawIDToken, + IDToken: getIDToken(token), RefreshToken: token.RefreshToken, CreatedAt: &created, ExpiresOn: &idToken.Expiry, diff --git a/providers/oidc.go b/providers/oidc.go index cdeee3b2..df133f4d 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -49,7 +49,7 @@ func (p *OIDCProvider) Redeem(ctx context.Context, redirectURL, code string) (*s return p.createSession(ctx, token, false) } -// EnrichSessionState is called after Redeem to allow providers to enrich session fields +// EnrichSession is called after Redeem to allow providers to enrich session fields // such as User, Email, Groups with provider specific API calls. func (p *OIDCProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error { if p.ProfileURL.String() == "" { @@ -61,7 +61,7 @@ func (p *OIDCProvider) EnrichSession(ctx context.Context, s *sessions.SessionSta // Try to get missing emails or groups from a profileURL if s.Email == "" || s.Groups == nil { - err := p.callProfileURL(ctx, s) + err := p.enrichFromProfileURL(ctx, s) if err != nil { logger.Errorf("Warning: Profile URL request failed: %v", err) } @@ -74,9 +74,9 @@ func (p *OIDCProvider) EnrichSession(ctx context.Context, s *sessions.SessionSta return nil } -// callProfileURL enriches a session's Email & Groups via the JSON response of +// enrichFromProfileURL enriches a session's Email & Groups via the JSON response of // an OIDC profile URL -func (p *OIDCProvider) callProfileURL(ctx context.Context, s *sessions.SessionState) error { +func (p *OIDCProvider) enrichFromProfileURL(ctx context.Context, s *sessions.SessionState) error { respJSON, err := requests.New(p.ProfileURL.String()). WithContext(ctx). WithHeaders(makeOIDCHeader(s.AccessToken)). @@ -91,22 +91,23 @@ func (p *OIDCProvider) callProfileURL(ctx context.Context, s *sessions.SessionSt s.Email = email } - if len(s.Groups) == 0 { - for _, group := range coerceArray(respJSON, p.GroupsClaim) { - formatted, err := formatGroup(group) - if err != nil { - logger.Errorf("Warning: unable to format group of type %s with error %s", - reflect.TypeOf(group), err) - continue - } - s.Groups = append(s.Groups, formatted) + if len(s.Groups) > 0 { + return nil + } + for _, group := range coerceArray(respJSON, p.GroupsClaim) { + formatted, err := formatGroup(group) + if err != nil { + logger.Errorf("Warning: unable to format group of type %s with error %s", + reflect.TypeOf(group), err) + continue } + s.Groups = append(s.Groups, formatted) } return nil } -// ValidateSessionState checks that the session's IDToken is still valid +// ValidateSession checks that the session's IDToken is still valid func (p *OIDCProvider) ValidateSession(ctx context.Context, s *sessions.SessionState) bool { _, err := p.Verifier.Verify(ctx, s.IDToken) return err == nil diff --git a/providers/provider_data.go b/providers/provider_data.go index d8c9312b..a9a41232 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -128,13 +128,13 @@ type OIDCClaims struct { func (p *ProviderData) verifyIDToken(ctx context.Context, token *oauth2.Token) (*oidc.IDToken, error) { rawIDToken := getIDToken(token) - if strings.TrimSpace(rawIDToken) != "" { - if p.Verifier == nil { - return nil, ErrMissingOIDCVerifier - } - return p.Verifier.Verify(ctx, rawIDToken) + if strings.TrimSpace(rawIDToken) == "" { + return nil, ErrMissingIDToken } - return nil, ErrMissingIDToken + if p.Verifier == nil { + return nil, ErrMissingOIDCVerifier + } + return p.Verifier.Verify(ctx, rawIDToken) } // buildSessionFromClaims uses IDToken claims to populate a fresh SessionState diff --git a/providers/util.go b/providers/util.go index 055d29db..e6fdc344 100644 --- a/providers/util.go +++ b/providers/util.go @@ -73,15 +73,15 @@ func getIDToken(token *oauth2.Token) string { // formatGroup coerces an OIDC groups claim into a string // If it is non-string, marshal it into JSON. func formatGroup(rawGroup interface{}) (string, error) { - group, ok := rawGroup.(string) - if !ok { - jsonGroup, err := json.Marshal(rawGroup) - if err != nil { - return "", err - } - group = string(jsonGroup) + if group, ok := rawGroup.(string); ok { + return group, nil } - return group, nil + + jsonGroup, err := json.Marshal(rawGroup) + if err != nil { + return "", err + } + return string(jsonGroup), nil } // coerceArray extracts a field from simplejson.Json that might be a