Repository: dexidp/dex
Pull request #1688 (2020-04-16 14:57:06)

feat: Add team groups support to bitbucket connector
## Description:
This PR provides support for team groups.  The only option added is `includeTeamGroups`. When the option value is `true`,  we make an additional request to Bitbucket API to get all team group names and combine them with the team name using `/`.

The groups claim in dex id_token looks like:
* Without option: `groups=["my_team"]`
* With option: `groups=["my_team", "my_team/administrators", "my_team/members"]`

## Motivation:
I started working on it because there was no chance to do authn/authz management in the frames of one team. This behavior is something that other connectors already has (github, gitlab).

## Concerns: 
My only concern is sending a request to deprecated API to get team groups. However, Atlassian said, that `/groups` endpoint would be supported until the equal method would appear in 2.0 API.

crytome1995: I really like this, and need this, but I can't seem to get the groups to be added to the claim. I believe this is due to the comparison of the original whitelisted teams in config compared to what is provided after group concatenation?
nabokihms: @crytome1995 to make `includeTeamGroups: true` works with teams filter correctly, you should add additional entries to your filter like:
```
includeTeamGroups: true
teams: ["my_team", "my_team/everyone", "my_team/administrators"]
```
This behavior helps to filter unnecessary groups from the result ID token.
nabokihms: I just rebased the branch to activate new CI jobs.
sagikazarmark: @nabokihms Thanks! The PR looks OK.

Regarding the filter: I believe other connectors (GitHub, GitLab) behave the same way, but what pains me is that config looks differently (again). 😢 
nabokihms: In this particular case, we can avoid adding an option. We just need to inform users that default behavior will change (I'm not a big fan of breaking changes, but it has its pros and cons). By the way, I am looking forward to your decision about this PR :)

P.S.:
About difference in configs... It's hard to keep connectors flexible enough without kind specific parameters.

In my opinion, it will be good to move some options from providers config to a higher level, like baseURL, clientID, clientSecret, etc, and use the config section only for unique connector options.
sagikazarmark: I'm actually a huge fan of how it is done in GitHub: https://github.com/banzaicloud/pipeline/blob/cb95d36/etc/config/dex.yml.dist#L104-L113

It's well structured, easy to understand.

In most of the cases just a concept or some rules to follow would be enough. But since there is no such thing, everyone just invents their own solutions.
sagikazarmark: Also, there is this: #1635
sagikazarmark: @nabokihms if you rebase this PR I'll merge it as well. Thanks!

Patch:
diff --git a/Documentation/connectors/bitbucketcloud.md b/Documentation/connectors/bitbucketcloud.md
index 2267079a..1e23bf30 100644
--- a/Documentation/connectors/bitbucketcloud.md
+++ b/Documentation/connectors/bitbucketcloud.md
@@ -31,4 +31,8 @@ connectors:
     # 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
+    # Optional parameter to include team groups.
+    # If enabled, the groups claim of dex id_token will looks like this:
+    # ["my_team", "my_team/administrators", "my_team/members"]
+    includeTeamGroups: true
 ```
diff --git a/connector/bitbucketcloud/bitbucketcloud.go b/connector/bitbucketcloud/bitbucketcloud.go
index ef942ae2..039084fe 100644
--- a/connector/bitbucketcloud/bitbucketcloud.go
+++ b/connector/bitbucketcloud/bitbucketcloud.go
@@ -21,7 +21,8 @@ import (
 
 const (
 	apiURL = "https://api.bitbucket.org/2.0"
-
+	// Switch to API v2.0 when the Atlassian platform services are fully available in Bitbucket
+	legacyAPIURL = "https://api.bitbucket.org/1.0"
 	// Bitbucket requires this scope to access '/user' API endpoints.
 	scopeAccount = "account"
 	// Bitbucket requires this scope to access '/user/emails' API endpoints.
@@ -33,21 +34,24 @@ const (
 
 // Config holds configuration options for Bitbucket logins.
 type Config struct {
-	ClientID     string   `json:"clientID"`
-	ClientSecret string   `json:"clientSecret"`
-	RedirectURI  string   `json:"redirectURI"`
-	Teams        []string `json:"teams"`
+	ClientID          string   `json:"clientID"`
+	ClientSecret      string   `json:"clientSecret"`
+	RedirectURI       string   `json:"redirectURI"`
+	Teams             []string `json:"teams"`
+	IncludeTeamGroups bool     `json:"includeTeamGroups,omitempty"`
 }
 
 // Open returns a strategy for logging in through Bitbucket.
-func (c *Config) Open(id string, logger log.Logger) (connector.Connector, error) {
+func (c *Config) Open(_ string, logger log.Logger) (connector.Connector, error) {
 	b := bitbucketConnector{
-		redirectURI:  c.RedirectURI,
-		teams:        c.Teams,
-		clientID:     c.ClientID,
-		clientSecret: c.ClientSecret,
-		apiURL:       apiURL,
-		logger:       logger,
+		redirectURI:       c.RedirectURI,
+		teams:             c.Teams,
+		clientID:          c.ClientID,
+		clientSecret:      c.ClientSecret,
+		includeTeamGroups: c.IncludeTeamGroups,
+		apiURL:            apiURL,
+		legacyAPIURL:      legacyAPIURL,
+		logger:            logger,
 	}
 
 	return &b, nil
@@ -71,10 +75,13 @@ type bitbucketConnector struct {
 	clientSecret string
 	logger       log.Logger
 	apiURL       string
+	legacyAPIURL string
 
 	// the following are used only for tests
 	hostName   string
 	httpClient *http.Client
+
+	includeTeamGroups bool
 }
 
 // groupsRequired returns whether dex requires Bitbucket's 'team' scope.
@@ -396,9 +403,39 @@ func (b *bitbucketConnector) userTeams(ctx context.Context, client *http.Client)
 		}
 	}
 
+	if b.includeTeamGroups {
+		for _, team := range teams {
+			teamGroups, err := b.userTeamGroups(ctx, client, team)
+			if err != nil {
+				return nil, fmt.Errorf("bitbucket: %v", err)
+			}
+			teams = append(teams, teamGroups...)
+		}
+	}
+
 	return teams, nil
 }
 
+type group struct {
+	Slug string `json:"slug"`
+}
+
+func (b *bitbucketConnector) userTeamGroups(ctx context.Context, client *http.Client, teamName string) ([]string, error) {
+	var teamGroups []string
+	apiURL := b.legacyAPIURL + "/groups/" + teamName
+
+	var response []group
+	if err := get(ctx, client, apiURL, &response); err != nil {
+		return nil, fmt.Errorf("get user team %q groups: %v", teamName, err)
+	}
+
+	for _, group := range response {
+		teamGroups = append(teamGroups, teamName+"/"+group.Slug)
+	}
+
+	return teamGroups, nil
+}
+
 // get creates a "GET `apiURL`" request with context, sends the request using
 // the client, and decodes the resulting response body into v.
 // Any errors encountered when building requests, sending requests, and


Test patch:
diff --git a/connector/bitbucketcloud/bitbucketcloud_test.go b/connector/bitbucketcloud/bitbucketcloud_test.go
index c9990d48..3d984a8f 100644
--- a/connector/bitbucketcloud/bitbucketcloud_test.go
+++ b/connector/bitbucketcloud/bitbucketcloud_test.go
@@ -29,9 +29,12 @@ func TestUserGroups(t *testing.T) {
 
 	s := newTestServer(map[string]interface{}{
 		"/user/permissions/teams": teamsResponse,
+		"/groups/team-1":          []group{{Slug: "administrators"}, {Slug: "members"}},
+		"/groups/team-2":          []group{{Slug: "everyone"}},
+		"/groups/team-3":          []group{},
 	})
 
-	connector := bitbucketConnector{apiURL: s.URL}
+	connector := bitbucketConnector{apiURL: s.URL, legacyAPIURL: s.URL}
 	groups, err := connector.userTeams(context.Background(), newClient())
 
 	expectNil(t, err)
@@ -41,6 +44,19 @@ func TestUserGroups(t *testing.T) {
 		"team-3",
 	})
 
+	connector.includeTeamGroups = true
+	groups, err = connector.userTeams(context.Background(), newClient())
+
+	expectNil(t, err)
+	expectEquals(t, groups, []string{
+		"team-1",
+		"team-2",
+		"team-3",
+		"team-1/administrators",
+		"team-1/members",
+		"team-2/everyone",
+	})
+
 	s.Close()
 }