From d26e23c16f64c7668a63d5be8284877e5954da1b Mon Sep 17 00:00:00 2001 From: Ed Tan Date: Thu, 4 Oct 2018 12:29:36 -0400 Subject: [PATCH] Make suggested code changes --- Documentation/connectors/bitbucket.md | 8 +++--- connector/bitbucket/bitbucket.go | 36 +++++++++++++-------------- connector/bitbucket/bitbucket_test.go | 12 ++++----- server/server.go | 24 +++++++++--------- 4 files changed, 41 insertions(+), 39 deletions(-) diff --git a/Documentation/connectors/bitbucket.md b/Documentation/connectors/bitbucket.md index f0746689..71754bee 100644 --- a/Documentation/connectors/bitbucket.md +++ b/Documentation/connectors/bitbucket.md @@ -14,7 +14,7 @@ The following is an example of a configuration for `examples/config-dev.yaml`: ```yaml connectors: -- type: bitbucket +- type: bitbucket-cloud # Required field for connector id. id: bitbucket # Required field for connector name. @@ -22,9 +22,11 @@ connectors: config: # Credentials can be string literals or pulled from the environment. clientID: $BITBUCKET_CLIENT_ID - clientSecret: BITBUCKET_CLIENT_SECRET + clientSecret: $BITBUCKET_CLIENT_SECRET redirectURI: http://127.0.0.1:5556/dex/callback - # Optional teams, communicated through the "groups" scope. + # Optional teams whitelist, communicated through the "groups" scope. + # If `teams` is omitted, all of the user's Bitbucket teams are returned when the groups scope is present. + # If `teams` is provided, this acts as a whitelist - only the user's Bitbucket teams that are in the configured `teams` below will go into the groups claim. Conversely, if the user is not in any of the configured `teams`, the user will not be authenticated. teams: - my-team ``` diff --git a/connector/bitbucket/bitbucket.go b/connector/bitbucket/bitbucket.go index 4e3620d3..0b3b1ba4 100644 --- a/connector/bitbucket/bitbucket.go +++ b/connector/bitbucket/bitbucket.go @@ -1,5 +1,5 @@ -// Package bitbucket provides authentication strategies using Bitbucket. -package bitbucket +// Package bitbucketcloud provides authentication strategies using Bitbucket Cloud. +package bitbucketcloud import ( "context" @@ -71,8 +71,7 @@ type bitbucketConnector struct { clientID string clientSecret string logger logrus.FieldLogger - // apiURL defaults to "https://api.bitbucket.org/2.0" - apiURL string + apiURL string // the following are used only for tests hostName string @@ -294,11 +293,11 @@ func (b *bitbucketConnector) user(ctx context.Context, client *http.Client) (use ) if err = get(ctx, client, b.apiURL+"/user", &u); err != nil { - return u, err + return user{}, err } if u.Email, err = b.userEmail(ctx, client); err != nil { - return u, err + return user{}, err } return u, nil @@ -309,7 +308,6 @@ func (b *bitbucketConnector) user(ctx context.Context, client *http.Client) (use type userEmail struct { IsPrimary bool `json:"is_primary"` IsConfirmed bool `json:"is_confirmed"` - Type string `json:"type"` Email string `json:"email"` } @@ -356,7 +354,7 @@ func (b *bitbucketConnector) getGroups(ctx context.Context, client *http.Client, if len(b.teams) > 0 { filteredTeams := filterTeams(bitbucketTeams, b.teams) if len(filteredTeams) == 0 { - return nil, fmt.Errorf("bitbucket: user %q not in required teams", userLogin) + return nil, fmt.Errorf("bitbucket: user %q is not in any of the required teams", userLogin) } return filteredTeams, nil } else if groupScope { @@ -367,23 +365,22 @@ func (b *bitbucketConnector) getGroups(ctx context.Context, client *http.Client, } // Filter the users' team memberships by 'teams' from config. -func filterTeams(userTeams, configTeams []string) (teams []string) { +func filterTeams(userTeams, configTeams []string) []string { + teams := []string{} teamFilter := make(map[string]struct{}) for _, team := range configTeams { - if _, ok := teamFilter[team]; !ok { - teamFilter[team] = struct{}{} - } + teamFilter[team] = struct{}{} } for _, team := range userTeams { if _, ok := teamFilter[team]; ok { teams = append(teams, team) } } - return + return teams } type team struct { - Username string `json:"username"` // Username is actually the team name + Name string `json:"username"` // The "username" from Bitbucket Cloud is actually the team name here } type userTeamsResponse struct { @@ -392,7 +389,10 @@ type userTeamsResponse struct { } func (b *bitbucketConnector) userTeams(ctx context.Context, client *http.Client) ([]string, error) { - apiURL, teams := b.apiURL+"/teams?role=member", []string{} + + var teams []string + apiURL := b.apiURL + "/teams?role=member" + for { // https://developer.atlassian.com/bitbucket/api/2/reference/resource/teams var response userTeamsResponse @@ -402,7 +402,7 @@ func (b *bitbucketConnector) userTeams(ctx context.Context, client *http.Client) } for _, team := range response.Values { - teams = append(teams, team.Username) + teams = append(teams, team.Name) } if response.Next == nil { @@ -432,13 +432,13 @@ func get(ctx context.Context, client *http.Client, apiURL string, v interface{}) if resp.StatusCode != http.StatusOK { body, err := ioutil.ReadAll(resp.Body) if err != nil { - return fmt.Errorf("bitbucket: read body: %v", err) + return fmt.Errorf("bitbucket: read body: %s: %v", resp.Status, err) } return fmt.Errorf("%s: %s", resp.Status, body) } if err := json.NewDecoder(resp.Body).Decode(v); err != nil { - return fmt.Errorf("failed to decode response: %v", err) + return fmt.Errorf("bitbucket: failed to decode response: %v", err) } return nil diff --git a/connector/bitbucket/bitbucket_test.go b/connector/bitbucket/bitbucket_test.go index b5d96266..b9f4ba08 100644 --- a/connector/bitbucket/bitbucket_test.go +++ b/connector/bitbucket/bitbucket_test.go @@ -1,4 +1,4 @@ -package bitbucket +package bitbucketcloud import ( "context" @@ -22,9 +22,9 @@ func TestUserGroups(t *testing.T) { PageLen: 10, }, Values: []team{ - {Username: "team-1"}, - {Username: "team-2"}, - {Username: "team-3"}, + {Name: "team-1"}, + {Name: "team-2"}, + {Name: "team-3"}, }, } @@ -113,12 +113,12 @@ func newClient() *http.Client { func expectNil(t *testing.T, a interface{}) { if a != nil { - t.Errorf("Expected %+v to equal nil", a) + t.Fatalf("Expected %+v to equal nil", a) } } func expectEquals(t *testing.T, a interface{}, b interface{}) { if !reflect.DeepEqual(a, b) { - t.Errorf("Expected %+v to equal %+v", a, b) + t.Fatalf("Expected %+v to equal %+v", a, b) } } diff --git a/server/server.go b/server/server.go index a2ca3e18..adf872eb 100644 --- a/server/server.go +++ b/server/server.go @@ -24,7 +24,7 @@ import ( "github.com/dexidp/dex/connector" "github.com/dexidp/dex/connector/authproxy" - "github.com/dexidp/dex/connector/bitbucket" + "github.com/dexidp/dex/connector/bitbucketcloud" "github.com/dexidp/dex/connector/github" "github.com/dexidp/dex/connector/gitlab" "github.com/dexidp/dex/connector/ldap" @@ -430,17 +430,17 @@ type ConnectorConfig interface { // ConnectorsConfig variable provides an easy way to return a config struct // depending on the connector type. var ConnectorsConfig = map[string]func() ConnectorConfig{ - "mockCallback": func() ConnectorConfig { return new(mock.CallbackConfig) }, - "mockPassword": func() ConnectorConfig { return new(mock.PasswordConfig) }, - "ldap": func() ConnectorConfig { return new(ldap.Config) }, - "github": func() ConnectorConfig { return new(github.Config) }, - "gitlab": func() ConnectorConfig { return new(gitlab.Config) }, - "oidc": func() ConnectorConfig { return new(oidc.Config) }, - "saml": func() ConnectorConfig { return new(saml.Config) }, - "authproxy": func() ConnectorConfig { return new(authproxy.Config) }, - "linkedin": func() ConnectorConfig { return new(linkedin.Config) }, - "microsoft": func() ConnectorConfig { return new(microsoft.Config) }, - "bitbucket": func() ConnectorConfig { return new(bitbucket.Config) }, + "mockCallback": func() ConnectorConfig { return new(mock.CallbackConfig) }, + "mockPassword": func() ConnectorConfig { return new(mock.PasswordConfig) }, + "ldap": func() ConnectorConfig { return new(ldap.Config) }, + "github": func() ConnectorConfig { return new(github.Config) }, + "gitlab": func() ConnectorConfig { return new(gitlab.Config) }, + "oidc": func() ConnectorConfig { return new(oidc.Config) }, + "saml": func() ConnectorConfig { return new(saml.Config) }, + "authproxy": func() ConnectorConfig { return new(authproxy.Config) }, + "linkedin": func() ConnectorConfig { return new(linkedin.Config) }, + "microsoft": func() ConnectorConfig { return new(microsoft.Config) }, + "bitbucket-cloud": func() ConnectorConfig { return new(bitbucketcloud.Config) }, // Keep around for backwards compatibility. "samlExperimental": func() ConnectorConfig { return new(saml.Config) }, }