From 15c447cd9097c5ad18b5148a85d78d4898266577 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Fri, 19 Jul 2024 14:00:13 -0700 Subject: [PATCH 01/13] Add rate limit annotations to groups --- pkg/connector/group.go | 82 ++++++++++++++++++++++++++++------------ pkg/connector/helpers.go | 11 ++++++ 2 files changed, 69 insertions(+), 24 deletions(-) diff --git a/pkg/connector/group.go b/pkg/connector/group.go index 36487f34..4bd5527b 100644 --- a/pkg/connector/group.go +++ b/pkg/connector/group.go @@ -5,12 +5,12 @@ import ( "fmt" "github.com/conductorone/baton-confluence/pkg/connector/client" - v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/pagination" - ent "github.com/conductorone/baton-sdk/pkg/types/entitlement" - grant "github.com/conductorone/baton-sdk/pkg/types/grant" - res "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/conductorone/baton-sdk/pkg/types/entitlement" + "github.com/conductorone/baton-sdk/pkg/types/grant" + "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" ) @@ -35,9 +35,9 @@ func groupResource(ctx context.Context, group *client.ConfluenceGroup) (*v2.Reso "group_type": group.Type, } - groupTraitOptions := []res.GroupTraitOption{res.WithGroupProfile(profile)} + groupTraitOptions := []resource.GroupTraitOption{resource.WithGroupProfile(profile)} - resource, err := res.NewGroupResource( + newGroupResource, err := resource.NewGroupResource( group.Name, resourceTypeGroup, group.Id, @@ -47,10 +47,19 @@ func groupResource(ctx context.Context, group *client.ConfluenceGroup) (*v2.Reso return nil, err } - return resource, nil + return newGroupResource, nil } -func (o *groupResourceType) List(ctx context.Context, resourceId *v2.ResourceId, pt *pagination.Token) ([]*v2.Resource, string, annotations.Annotations, error) { +func (o *groupResourceType) List( + ctx context.Context, + resourceId *v2.ResourceId, + pt *pagination.Token, +) ( + []*v2.Resource, + string, + annotations.Annotations, + error, +) { bag := &pagination.Bag{} err := bag.Unmarshal(pt.Token) if err != nil { @@ -61,9 +70,10 @@ func (o *groupResourceType) List(ctx context.Context, resourceId *v2.ResourceId, ResourceTypeID: resourceTypeGroup.Id, }) } - groups, token, err := o.client.GetGroups(ctx, bag.PageToken(), ResourcesPageSize) + groups, token, ratelimitData, err := o.client.GetGroups(ctx, bag.PageToken(), ResourcesPageSize) + outputAnnotations := WithRateLimitAnnotations(ratelimitData) if err != nil { - return nil, "", nil, err + return nil, "", outputAnnotations, err } rv := make([]*v2.Resource, 0, len(groups)) @@ -72,7 +82,7 @@ func (o *groupResourceType) List(ctx context.Context, resourceId *v2.ResourceId, gr, err := groupResource(ctx, &groupCopy) if err != nil { - return nil, "", nil, err + return nil, "", outputAnnotations, err } rv = append(rv, gr) @@ -80,22 +90,31 @@ func (o *groupResourceType) List(ctx context.Context, resourceId *v2.ResourceId, nextPage, err := bag.NextToken(token) if err != nil { - return nil, "", nil, err + return nil, "", outputAnnotations, err } - return rv, nextPage, nil, nil + return rv, nextPage, outputAnnotations, nil } -func (o *groupResourceType) Entitlements(ctx context.Context, resource *v2.Resource, _ *pagination.Token) ([]*v2.Entitlement, string, annotations.Annotations, error) { +func (o *groupResourceType) Entitlements( + ctx context.Context, + resource *v2.Resource, + _ *pagination.Token, +) ( + []*v2.Entitlement, + string, + annotations.Annotations, + error, +) { var rv []*v2.Entitlement - assignmentOptions := []ent.EntitlementOption{ - ent.WithGrantableTo(resourceTypeUser), - ent.WithDisplayName(fmt.Sprintf("%s Group Member", resource.DisplayName)), - ent.WithDescription(fmt.Sprintf("Is member of the %s group in Confluence", resource.DisplayName)), + assignmentOptions := []entitlement.EntitlementOption{ + entitlement.WithGrantableTo(resourceTypeUser), + entitlement.WithDisplayName(fmt.Sprintf("%s Group Member", resource.DisplayName)), + entitlement.WithDescription(fmt.Sprintf("Is member of the %s group in Confluence", resource.DisplayName)), } - rv = append(rv, ent.NewAssignmentEntitlement( + rv = append(rv, entitlement.NewAssignmentEntitlement( resource, groupMemberEntitlement, assignmentOptions..., @@ -104,7 +123,16 @@ func (o *groupResourceType) Entitlements(ctx context.Context, resource *v2.Resou return rv, "", nil, nil } -func (o *groupResourceType) Grants(ctx context.Context, resource *v2.Resource, pt *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { +func (o *groupResourceType) Grants( + ctx context.Context, + resource *v2.Resource, + pt *pagination.Token, +) ( + []*v2.Grant, + string, + annotations.Annotations, + error, +) { l := ctxzap.Extract(ctx) bag := &pagination.Bag{} err := bag.Unmarshal(pt.Token) @@ -117,9 +145,15 @@ func (o *groupResourceType) Grants(ctx context.Context, resource *v2.Resource, p }) } - users, token, err := o.client.GetGroupMembers(ctx, bag.PageToken(), ResourcesPageSize, resource.DisplayName) + users, token, ratelimitData, err := o.client.GetGroupMembers( + ctx, + bag.PageToken(), + ResourcesPageSize, + resource.Id.Resource, + ) + outputAnnotations := WithRateLimitAnnotations(ratelimitData) if err != nil { - return nil, "", nil, err + return nil, "", outputAnnotations, err } var rv []*v2.Grant @@ -141,9 +175,9 @@ func (o *groupResourceType) Grants(ctx context.Context, resource *v2.Resource, p nextPage, err := bag.NextToken(token) if err != nil { - return nil, "", nil, err + return nil, "", outputAnnotations, err } - return rv, nextPage, nil, nil + return rv, nextPage, outputAnnotations, nil } func groupBuilder(client *client.ConfluenceClient) *groupResourceType { diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index da1ed38d..7991af83 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -12,3 +12,14 @@ func annotationsForUserResourceType() annotations.Annotations { annos.Update(&v2.SkipEntitlementsAndGrants{}) return annos } + +func WithRateLimitAnnotations( + ratelimitDescriptionAnnotations ...*v2.RateLimitDescription, +) annotations.Annotations { + outputAnnotations := annotations.Annotations{} + for _, annotation := range ratelimitDescriptionAnnotations { + outputAnnotations.Append(annotation) + } + + return outputAnnotations +} From 0cefb773e62d88259469fd0f66218ac396400d76 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Fri, 19 Jul 2024 14:01:06 -0700 Subject: [PATCH 02/13] Refactor users list to use pagination and rate limits --- pkg/connector/client/confluence.go | 435 +++++++++++++++++------------ pkg/connector/client/models.go | 26 +- pkg/connector/connector.go | 1 + pkg/connector/user.go | 182 ++++++++++-- pkg/connector/user_test.go | 3 +- 5 files changed, 442 insertions(+), 205 deletions(-) diff --git a/pkg/connector/client/confluence.go b/pkg/connector/client/confluence.go index f7f25fb9..c311a0cc 100644 --- a/pkg/connector/client/confluence.go +++ b/pkg/connector/client/confluence.go @@ -8,17 +8,21 @@ import ( "io" "net/http" "net/url" + "slices" "strconv" - "time" + "strings" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/helpers" "github.com/conductorone/baton-sdk/pkg/uhttp" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) const ( - maxResults = 50 - minRatelimitWait = 1 * time.Second // Minimum time to wait after a request was ratelimited before trying again - maxRatelimitWait = (2 * time.Minute) + (30 * time.Second) // Maximum time to wait after a request was ratelimited before erroring - + maxResults = 50 currentUserUrlPath = "/wiki/rest/api/user/current" GroupsListUrlPath = "/wiki/rest/api/group" getUsersByGroupIdUrlPath = "/wiki/rest/api/group/%s/membersByGroupId" @@ -42,10 +46,10 @@ func (r *RequestError) Error() string { } type ConfluenceClient struct { - httpClient *http.Client - user string - apiKey string - apiBase *url.URL + user string + apiKey string + apiBase *url.URL + wrapper *uhttp.BaseHttpClient } // fallBackToHTTPS checks to domain and tacks on "https://" if no scheme is @@ -66,7 +70,7 @@ func fallBackToHTTPS(domain string) (*url.URL, error) { } func NewConfluenceClient(ctx context.Context, user, apiKey, domain string) (*ConfluenceClient, error) { - apiBase, err := fallBackToHTTPS(fmt.Sprintf("%s/wiki/rest/api/", domain)) + apiBase, err := fallBackToHTTPS(domain) if err != nil { return nil, err } @@ -77,25 +81,26 @@ func NewConfluenceClient(ctx context.Context, user, apiKey, domain string) (*Con } return &ConfluenceClient{ - httpClient: httpClient, - apiBase: apiBase, - user: user, - apiKey: apiKey, + apiBase: apiBase, + apiKey: apiKey, + user: user, + wrapper: uhttp.NewBaseHttpClient(httpClient), }, nil } func (c *ConfluenceClient) Verify(ctx context.Context) error { - u, err := c.genURLNonPaginated("user/current") + currentUserUrl, err := c.genURLNonPaginated(currentUserUrlPath) if err != nil { return err } - var resp *ConfluenceUser - if err := c.get(ctx, u, &resp); err != nil { + var response *ConfluenceUser + _, err = c.get(ctx, currentUserUrl, &response) + if err != nil { return err } - currentUser := resp.AccountId + currentUser := response.AccountId if currentUser == "" { return errors.New("failed to find new user") } @@ -103,224 +108,262 @@ func (c *ConfluenceClient) Verify(ctx context.Context) error { return nil } -func (c *ConfluenceClient) GetUsers(ctx context.Context, pageToken string, pageSize int) ([]ConfluenceUser, string, error) { - // There is no api to get all users, so get all groups then all members of each group. - // We also have to internally handle paging, due to the multiple layers of requests. +func isThereAnotherPage(links ConfluenceLink) bool { + return links.Next != "" +} - groups := make([]ConfluenceGroup, 0) +func (c *ConfluenceClient) GetGroups( + ctx context.Context, + pageToken string, + pageSize int, +) ( + []ConfluenceGroup, + string, + *v2.RateLimitDescription, + error, +) { + groupsUrl, err := c.genURL(pageToken, pageSize, GroupsListUrlPath) + if err != nil { + return nil, "", nil, err + } - // Get all groups - for { - u, err := c.genURL(pageToken, pageSize, "group") - if err != nil { - return nil, "", err - } + var response *confluenceGroupList + ratelimitData, err := c.get(ctx, groupsUrl, &response) + if err != nil { + return nil, "", ratelimitData, err + } - var resp *confluenceGroupList - if err := c.get(ctx, u, &resp); err != nil { - return nil, "", err - } + groups := response.Results - groupPage := resp.Results - if len(groupPage) == 0 { - break - } - groups = append(groups, groupPage...) - pageToken = incToken(pageToken, len(groupPage)) + if !isThereAnotherPage(response.Links) { + return groups, "", ratelimitData, nil } - if len(groups) == 0 { - return []ConfluenceUser{}, "", nil - } + token := incToken(pageToken, len(groups)) - userMap := make(map[string]ConfluenceUser) + return groups, token, ratelimitData, nil +} - // Get members of each group - for _, group := range groups { - users := make([]ConfluenceUser, 0) - pageToken = "" - for { - u, err := c.genURL(pageToken, pageSize, "group/member?name="+group.Name) - if err != nil { - return nil, "", err - } +func (c *ConfluenceClient) GetGroupMembers( + ctx context.Context, + pageToken string, + pageSize int, + groupId string, +) ( + []ConfluenceUser, + string, + *v2.RateLimitDescription, + error, +) { + getUsersUrl, err := c.genURL(pageToken, pageSize, fmt.Sprintf(getUsersByGroupIdUrlPath, groupId)) + if err != nil { + return nil, "", nil, err + } - var resp *confluenceUserList - if err := c.get(ctx, u, &resp); err != nil { - return nil, "", err - } + var response *confluenceUserList + ratelimitData, err := c.get(ctx, getUsersUrl, &response) + if err != nil { + return nil, "", ratelimitData, err + } - userPage := resp.Results - if len(userPage) == 0 { - break - } + users := response.Results - users = append(users, userPage...) + if !isThereAnotherPage(response.Links) { + return users, "", ratelimitData, nil + } - pageToken = incToken(pageToken, len(userPage)) - } + token := incToken(pageToken, len(users)) - // De-dupe users accross groups - for _, user := range users { - if _, ok := userMap[user.AccountId]; !ok { - userMap[user.AccountId] = user - } - } + return users, token, ratelimitData, nil +} + +func (c *ConfluenceClient) AddUserToGroup( + ctx context.Context, + accountID string, + groupId string, +) (*v2.RateLimitDescription, error) { + getUsersUrl, err := c.genURLNonPaginated( + fmt.Sprintf( + addUsersToGroupUrlPath, + groupId, + ), + ) + if err != nil { + return nil, err } - allUsers := make([]ConfluenceUser, 0) - for _, user := range userMap { - allUsers = append(allUsers, user) + bodyBytes, err := json.Marshal( + AddUserToGroupRequestBody{ + AccountId: accountID, + }, + ) + if err != nil { + return nil, err } - return allUsers, "", nil + body := strings.NewReader(string(bodyBytes)) + ratelimitData, err := c.post(ctx, getUsersUrl, nil, body) + if err != nil { + return ratelimitData, err + } + return ratelimitData, nil } -func (c *ConfluenceClient) GetGroups(ctx context.Context, pageToken string, pageSize int) ([]ConfluenceGroup, string, error) { - u, err := c.genURL(pageToken, pageSize, "group") +func (c *ConfluenceClient) RemoveUserFromGroup( + ctx context.Context, + accountID string, + groupId string, +) (*v2.RateLimitDescription, error) { + getUsersUrl, err := c.genURLNonPaginated( + fmt.Sprintf( + removeUsersFromGroupUrlPath, + groupId, + accountID, + ), + ) if err != nil { - return nil, "", err + return nil, err } - var resp *confluenceGroupList - if err := c.get(ctx, u, &resp); err != nil { - return nil, "", err + ratelimitData, err := c.delete(ctx, getUsersUrl, nil) + if err != nil { + return ratelimitData, err } + return ratelimitData, nil +} - groups := resp.Results - - if len(groups) == 0 { - return groups, "", nil - } +func (c *ConfluenceClient) get( + ctx context.Context, + getUrl *url.URL, + target interface{}, +) (*v2.RateLimitDescription, error) { + return c.makeRequest(ctx, getUrl, target, http.MethodGet, nil) +} - token := incToken(pageToken, len(groups)) +func (c *ConfluenceClient) post( + ctx context.Context, + postUrl *url.URL, + target interface{}, + requestBody io.Reader, +) (*v2.RateLimitDescription, error) { + return c.makeRequest(ctx, postUrl, target, http.MethodPost, requestBody) +} - return groups, token, nil +func (c *ConfluenceClient) delete( + ctx context.Context, + deleteUrl *url.URL, + target interface{}, +) (*v2.RateLimitDescription, error) { + return c.makeRequest(ctx, deleteUrl, target, http.MethodDelete, nil) } -func (c *ConfluenceClient) GetGroupMembers(ctx context.Context, pageToken string, pageSize int, group string) ([]ConfluenceUser, string, error) { - u, err := c.genURL(pageToken, pageSize, "group/member?name="+group) +func (c *ConfluenceClient) makeRequest( + ctx context.Context, + url *url.URL, + target interface{}, + method string, + requestBody io.Reader, +) (*v2.RateLimitDescription, error) { + req, err := http.NewRequestWithContext(ctx, method, url.String(), requestBody) if err != nil { - return nil, "", err + return nil, err } - var resp *confluenceUserList - if err := c.get(ctx, u, &resp); err != nil { - return nil, "", err - } + req.SetBasicAuth(c.user, c.apiKey) - users := resp.Results + ratelimitData := v2.RateLimitDescription{} + + response, err := c.wrapper.Do( + req, + WithConfluenceRatelimitData(&ratelimitData), + uhttp.WithJSONResponse(target), + ) + if err == nil { + return &ratelimitData, nil + } + if response == nil { + return nil, err + } + defer response.Body.Close() - if len(users) == 0 { - return users, "", nil + // If we get ratelimit data back (e.g. the "Retry-After" header) or a + // "ratelimit-like" status code, then return a recoverable gRPC code. + if isRatelimited(ratelimitData.Status, response.StatusCode) { + return &ratelimitData, status.Error(codes.Unavailable, response.Status) } - token := incToken(pageToken, len(users)) + // If it's some other error, it is unrecoverable. + responseBody, err := io.ReadAll(response.Body) + if err != nil { + return nil, err + } - return users, token, nil + return nil, &RequestError{ + URL: url, + Status: response.StatusCode, + Body: string(responseBody), + } } -func (c *ConfluenceClient) get(ctx context.Context, u *url.URL, target interface{}) error { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil) +// genURLWithPaginationCursor uses Confluence Cloud's REST API v2 pagination scheme. +func (c *ConfluenceClient) genURLWithPaginationCursor( + path string, + pageSize int, + paginationCursor string, +) (*url.URL, error) { + parsed, err := url.Parse(path) if err != nil { - return err + return nil, fmt.Errorf("failed to parse request path '%s': %w", path, err) } - req.SetBasicAuth(c.user, c.apiKey) - - for { - resp, err := c.httpClient.Do(req) - if err != nil { - return err - } - defer resp.Body.Close() - - retryAfter := strToInt(resp.Header.Get("Retry-After")) - - switch resp.StatusCode { - case http.StatusOK: - if err := json.NewDecoder(resp.Body).Decode(target); err != nil { - return fmt.Errorf("failed to decode response body for '%s': %w", u, err) - } - return nil - case http.StatusTooManyRequests: - if err := wait(ctx, retryAfter); err != nil { - return fmt.Errorf("confluence-connector: failed to wait for retry on '%s': %w", u, err) - } - continue - case http.StatusServiceUnavailable: - // Per the docs: transient 5XX errors should be treated as 429/too-many-requests if they have a retry header. - // 503 errors were the only ones explicitly called out, but I guess it's possible for others too. - // https://developer.atlassian.com/cloud/confluence/rate-limiting/ - if retryAfter != 0 { - if err := wait(ctx, retryAfter); err != nil { - return fmt.Errorf("confluence-connector: failed to wait for retry on '%s': %w", u, err) - } - continue - } - } + parsedUrl := c.apiBase.ResolveReference(parsed) - body, err := io.ReadAll(resp.Body) - if err != nil { - return fmt.Errorf("error reading non-200 response body: %w", err) - } + maximum := pageSize + if maximum == 0 || maximum > maxResults { + maximum = maxResults + } - return &RequestError{ - URL: u, - Status: resp.StatusCode, - Body: string(body), - } + query := parsedUrl.Query() + if paginationCursor != "" { + query.Set("cursor", paginationCursor) } + query.Set("limit", strconv.Itoa(maximum)) + parsedUrl.RawQuery = query.Encode() + + return parsedUrl, nil } +// genURLNonPaginated adds the given URL path to the API base URL. func (c *ConfluenceClient) genURLNonPaginated(path string) (*url.URL, error) { parsed, err := url.Parse(path) if err != nil { return nil, fmt.Errorf("failed to parse request path '%s': %w", path, err) } - u := c.apiBase.ResolveReference(parsed) - return u, nil + parsedUrl := c.apiBase.ResolveReference(parsed) + return parsedUrl, nil } +// genURL adds `start` and `limit` query parameters to a URL. This pagination +// parameter is only used by the v1 REST API. func (c *ConfluenceClient) genURL(pageToken string, pageSize int, path string) (*url.URL, error) { parsed, err := url.Parse(path) if err != nil { return nil, fmt.Errorf("failed to parse request path '%s': %w", path, err) } - u := c.apiBase.ResolveReference(parsed) + parsedUrl := c.apiBase.ResolveReference(parsed) - max := pageSize - if max == 0 || max > maxResults { - max = maxResults + maximum := pageSize + if maximum == 0 || maximum > maxResults { + maximum = maxResults } - q := u.Query() - q.Set("start", pageToken) - q.Set("limit", strconv.Itoa(max)) - u.RawQuery = q.Encode() + query := parsedUrl.Query() + query.Set("start", pageToken) + query.Set("limit", strconv.Itoa(maximum)) + parsedUrl.RawQuery = query.Encode() - return u, nil -} - -func wait(ctx context.Context, retryAfter int) error { - now := time.Now() - resetAt := now.Add(time.Duration(retryAfter) * time.Second) - - // Wait must be within min/max window - d := resetAt.Sub(now) - if d < minRatelimitWait { - d = minRatelimitWait - } else if d > maxRatelimitWait { - d = maxRatelimitWait - } - - select { - case <-time.After(d): - return nil - case <-ctx.Done(): - return ctx.Err() - } + return parsedUrl, nil } func incToken(pageToken string, count int) string { @@ -341,3 +384,49 @@ func strToInt(s string) int { } return i } + +// extractPaginationCursor returns the query parameters from the "next" link in +// the list response. +func extractPaginationCursor(links ConfluenceLink) string { + parsedUrl, err := url.Parse(links.Next) + if err != nil { + return "" + } + return parsedUrl.Query().Get("cursor") +} + + +// WithConfluenceRatelimitData Per the docs: transient 5XX errors should be +// treated as 429/too-many-requests if they have a retry header. 503 errors were +// the only ones explicitly called out, but I guess it's possible for others too +// https://developer.atlassian.com/cloud/confluence/rate-limiting/ +func WithConfluenceRatelimitData(resource *v2.RateLimitDescription) uhttp.DoOption { + return func(response *uhttp.WrapperResponse) error { + rateLimitData, err := helpers.ExtractRateLimitData(response.StatusCode, &response.Header) + if err != nil { + return err + } + resource = rateLimitData + return nil + } +} + +func isRatelimited( + ratelimitStatus v2.RateLimitDescription_Status, + statusCode int, +) bool { + return slices.Contains( + []v2.RateLimitDescription_Status{ + v2.RateLimitDescription_STATUS_OVERLIMIT, + v2.RateLimitDescription_STATUS_ERROR, + }, + ratelimitStatus, + ) || slices.Contains( + []int{ + http.StatusTooManyRequests, + http.StatusGatewayTimeout, + http.StatusServiceUnavailable, + }, + statusCode, + ) +} diff --git a/pkg/connector/client/models.go b/pkg/connector/client/models.go index fbd96ac7..5ccc48d1 100644 --- a/pkg/connector/client/models.go +++ b/pkg/connector/client/models.go @@ -1,17 +1,18 @@ package client type ConfluenceUser struct { - AccountId string - AccountType string - DisplayName string - Email string + AccountId string `json:"accountId"` + AccountType string `json:"accountType"` + DisplayName string `json:"displayName"` + Email string `json:"email"` } type confluenceUserList struct { - Start int - Limit int - Size int - Results []ConfluenceUser + Start int `json:"start"` + Limit int `json:"limit"` + Size int `json:"size"` + Links ConfluenceLink `json:"_links"` + Results []ConfluenceUser `json:"results"` } type ConfluenceGroup struct { @@ -21,8 +22,9 @@ type ConfluenceGroup struct { } type confluenceGroupList struct { - Start int - Limit int - Size int - Results []ConfluenceGroup + Start int `json:"start"` + Limit int `json:"limit"` + Size int `json:"size"` + Links ConfluenceLink `json:"_links"` + Results []ConfluenceGroup `json:"results"` } diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index e7a56165..88f071da 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -13,6 +13,7 @@ import ( const ( accountTypeAtlassian = "atlassian" // user account type + accountTypeApp = "app" // bot account type ) var ( diff --git a/pkg/connector/user.go b/pkg/connector/user.go index ffea8abe..e07ecca1 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -2,12 +2,13 @@ package connector import ( "context" + "fmt" "github.com/conductorone/baton-confluence/pkg/connector/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/pagination" - resource "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" ) @@ -35,7 +36,7 @@ func userResource(ctx context.Context, user *client.ConfluenceUser) (*v2.Resourc resource.WithStatus(v2.UserTrait_Status_STATUS_ENABLED), } - resource, err := resource.NewUserResource( + newUserResource, err := resource.NewUserResource( user.DisplayName, resourceTypeUser, user.AccountId, @@ -45,40 +46,185 @@ func userResource(ctx context.Context, user *client.ConfluenceUser) (*v2.Resourc return nil, err } - return resource, nil + return newUserResource, nil } -func (o *userResourceType) List(ctx context.Context, _ *v2.ResourceId, pt *pagination.Token) ([]*v2.Resource, string, annotations.Annotations, error) { - l := ctxzap.Extract(ctx) +// parsePageToken given a marshalled pageToken as a string, return the pageToken +// bag and the current page number. +func parsePageToken( + pageToken string, + resourceID *v2.ResourceId, +) ( + *pagination.Bag, + string, + error, +) { + b := &pagination.Bag{} + err := b.Unmarshal(pageToken) + if err != nil { + return nil, "0", err + } + + if b.Current() == nil { + b.Push(pagination.PageState{ + ResourceTypeID: resourceID.ResourceType, + ResourceID: resourceID.Resource, + }) + } - users, _, err := o.client.GetUsers(ctx, "", ResourcesPageSize) + page := b.PageToken() + return b, page, nil +} + +func (o *userResourceType) List( + ctx context.Context, + _ *v2.ResourceId, + pToken *pagination.Token, +) ( + []*v2.Resource, + string, + annotations.Annotations, + error, +) { + logger := ctxzap.Extract(ctx) + logger.Debug("Starting Users List", zap.String("token", pToken.Token)) + + // There is no Confluence Cloud REST API to get all users, so get all groups + // and then all members of each group. + + // The second parameter here is "user", which acts as a default value. + bag, page, err := parsePageToken( + pToken.Token, + &v2.ResourceId{ResourceType: resourceTypeUser.Id}, + ) if err != nil { return nil, "", nil, err } - rv := make([]*v2.Resource, 0) - for _, user := range users { - if user.AccountType != accountTypeAtlassian { - l.Debug("confluence: user is not of type atlassian", zap.Any("user", user)) - continue + + outputResources := make([]*v2.Resource, 0) + var outputAnnotations annotations.Annotations + switch bag.ResourceTypeID() { + case resourceTypeUser.Id: + logger.Debug("Got a user from the bag", zap.String("page", page)) + if page == "" { + page = "0" + } + // Add a new page of groups + groups, nextToken, ratelimitData, err := o.client.GetGroups( + ctx, + page, + ResourcesPageSize, + ) + logger.Debug( + "Got groups", + zap.Int("len", len(groups)), + zap.String("nextToken", nextToken), + ) + outputAnnotations = WithRateLimitAnnotations(ratelimitData) + if err != nil { + return nil, "", outputAnnotations, err + } + + // Push next page to stack. (Short-circuits if token is "".) + err = bag.Next(nextToken) + if err != nil { + return nil, "", outputAnnotations, err } - userCopy := user - ur, err := userResource(ctx, &userCopy) + for _, group := range groups { + logger.Debug( + "adding a group to the bag", + zap.String("id", group.Id), + zap.String("name", group.Name), + ) + bag.Push( + pagination.PageState{ + ResourceTypeID: resourceTypeGroup.Id, + ResourceID: group.Id, + }, + ) + } + case resourceTypeGroup.Id: + currentState := bag.Current() + + start := currentState.Token + if start == "" { + start = "0" + } + logger.Debug( + "Got a group from the bag", + zap.String("start", start), + zap.String("group_id", currentState.ResourceID), + ) + + // Get users for this group. + users, nextToken, ratelimitData, err := o.client.GetGroupMembers( + ctx, + start, + ResourcesPageSize, + currentState.ResourceID, + ) + outputAnnotations = WithRateLimitAnnotations(ratelimitData) if err != nil { - return nil, "", nil, err + return nil, "", outputAnnotations, err } - rv = append(rv, ur) + // Push next page to stack. (Short-circuits if token is "".) + err = bag.Next(nextToken) + if err != nil { + return nil, "", outputAnnotations, err + } + + // Add users to output resources. There will be duplicates across groups. + for _, user := range users { + if user.AccountType != accountTypeAtlassian { + logger.Debug("confluence: user is not of type atlassian", zap.Any("user", user)) + continue + } + + userCopy := user + newUserResource, err := userResource(ctx, &userCopy) + if err != nil { + return nil, "", nil, err + } + + outputResources = append(outputResources, newUserResource) + } + default: + return nil, "", nil, fmt.Errorf("unexpected resource type while fetching list of users") + } + + pageToken, err := bag.Marshal() + if err != nil { + return nil, "", nil, err } - return rv, "", nil, nil + return outputResources, pageToken, outputAnnotations, nil } -func (o *userResourceType) Entitlements(_ context.Context, _ *v2.Resource, _ *pagination.Token) ([]*v2.Entitlement, string, annotations.Annotations, error) { +func (o *userResourceType) Entitlements( + _ context.Context, + _ *v2.Resource, + _ *pagination.Token, +) ( + []*v2.Entitlement, + string, + annotations.Annotations, + error, +) { return nil, "", nil, nil } -func (o *userResourceType) Grants(_ context.Context, _ *v2.Resource, _ *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { +func (o *userResourceType) Grants( + _ context.Context, + _ *v2.Resource, + _ *pagination.Token, +) ( + []*v2.Grant, + string, + annotations.Annotations, + error, +) { return nil, "", nil, nil } diff --git a/pkg/connector/user_test.go b/pkg/connector/user_test.go index bb5fad2b..b40e7043 100644 --- a/pkg/connector/user_test.go +++ b/pkg/connector/user_test.go @@ -56,8 +56,7 @@ func TestUsersList(t *testing.T) { require.NotNil(t, resources) // We expect there to be duplicates. - // require.Len(t, resources, 3) - require.Len(t, resources, 2) + require.Len(t, resources, 3) require.NotEmpty(t, resources[0].Id) }) } From 869ec4fd6caf5b23c03ac7f2a5b390ce03d831b7 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Fri, 19 Jul 2024 15:04:15 -0700 Subject: [PATCH 03/13] Add group provisioning --- pkg/connector/group.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/pkg/connector/group.go b/pkg/connector/group.go index 4bd5527b..a439825d 100644 --- a/pkg/connector/group.go +++ b/pkg/connector/group.go @@ -180,6 +180,33 @@ func (o *groupResourceType) Grants( return rv, nextPage, outputAnnotations, nil } +func (o *groupResourceType) Grant( + ctx context.Context, + principal *v2.Resource, + entitlement *v2.Entitlement, +) (annotations.Annotations, error) { + ratelimitData, err := o.client.AddUserToGroup( + ctx, + entitlement.Resource.Id.Resource, + principal.Id.Resource, + ) + outputAnnotations := WithRateLimitAnnotations(ratelimitData) + return outputAnnotations, err +} + +func (o *groupResourceType) Revoke( + ctx context.Context, + grant *v2.Grant, +) (annotations.Annotations, error) { + ratelimitData, err := o.client.RemoveUserFromGroup( + ctx, + grant.Entitlement.Resource.Id.Resource, + grant.Principal.Id.Resource, + ) + outputAnnotations := WithRateLimitAnnotations(ratelimitData) + return outputAnnotations, err +} + func groupBuilder(client *client.ConfluenceClient) *groupResourceType { return &groupResourceType{ resourceType: resourceTypeGroup, From 72f6d91d0b57ac33f8a02e9b19f9565f7a751a82 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Thu, 25 Jul 2024 12:02:56 -0700 Subject: [PATCH 04/13] fix lint --- pkg/connector/client/confluence.go | 41 ------------------------------ pkg/connector/client/models.go | 9 +++++++ pkg/connector/group.go | 2 +- 3 files changed, 10 insertions(+), 42 deletions(-) diff --git a/pkg/connector/client/confluence.go b/pkg/connector/client/confluence.go index c311a0cc..49c11360 100644 --- a/pkg/connector/client/confluence.go +++ b/pkg/connector/client/confluence.go @@ -15,8 +15,6 @@ import ( v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/helpers" "github.com/conductorone/baton-sdk/pkg/uhttp" - "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" - "go.uber.org/zap" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -305,34 +303,6 @@ func (c *ConfluenceClient) makeRequest( } } -// genURLWithPaginationCursor uses Confluence Cloud's REST API v2 pagination scheme. -func (c *ConfluenceClient) genURLWithPaginationCursor( - path string, - pageSize int, - paginationCursor string, -) (*url.URL, error) { - parsed, err := url.Parse(path) - if err != nil { - return nil, fmt.Errorf("failed to parse request path '%s': %w", path, err) - } - - parsedUrl := c.apiBase.ResolveReference(parsed) - - maximum := pageSize - if maximum == 0 || maximum > maxResults { - maximum = maxResults - } - - query := parsedUrl.Query() - if paginationCursor != "" { - query.Set("cursor", paginationCursor) - } - query.Set("limit", strconv.Itoa(maximum)) - parsedUrl.RawQuery = query.Encode() - - return parsedUrl, nil -} - // genURLNonPaginated adds the given URL path to the API base URL. func (c *ConfluenceClient) genURLNonPaginated(path string) (*url.URL, error) { parsed, err := url.Parse(path) @@ -385,17 +355,6 @@ func strToInt(s string) int { return i } -// extractPaginationCursor returns the query parameters from the "next" link in -// the list response. -func extractPaginationCursor(links ConfluenceLink) string { - parsedUrl, err := url.Parse(links.Next) - if err != nil { - return "" - } - return parsedUrl.Query().Get("cursor") -} - - // WithConfluenceRatelimitData Per the docs: transient 5XX errors should be // treated as 429/too-many-requests if they have a retry header. 503 errors were // the only ones explicitly called out, but I guess it's possible for others too diff --git a/pkg/connector/client/models.go b/pkg/connector/client/models.go index 5ccc48d1..d55f8777 100644 --- a/pkg/connector/client/models.go +++ b/pkg/connector/client/models.go @@ -1,5 +1,10 @@ package client +type ConfluenceLink struct { + Base string `json:"base"` + Next string `json:"next,omitempty"` +} + type ConfluenceUser struct { AccountId string `json:"accountId"` AccountType string `json:"accountType"` @@ -28,3 +33,7 @@ type confluenceGroupList struct { Links ConfluenceLink `json:"_links"` Results []ConfluenceGroup `json:"results"` } + +type AddUserToGroupRequestBody struct { + AccountId string `json:"accountId"` +} diff --git a/pkg/connector/group.go b/pkg/connector/group.go index a439825d..ca8e5d09 100644 --- a/pkg/connector/group.go +++ b/pkg/connector/group.go @@ -5,7 +5,7 @@ import ( "fmt" "github.com/conductorone/baton-confluence/pkg/connector/client" - "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/pagination" "github.com/conductorone/baton-sdk/pkg/types/entitlement" From 484d5b1e901c1a55b1e80bb1b178a70d3626b088 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Tue, 30 Jul 2024 12:21:19 -0700 Subject: [PATCH 05/13] Use User Search API to get users --- pkg/connector/client/confluence.go | 58 ++++++++-- pkg/connector/client/models.go | 17 ++- pkg/connector/client/path.go | 102 +++++++++++++++++ pkg/connector/user.go | 131 +++++----------------- pkg/connector/user_test.go | 2 +- test/fixtures/{users2.json => blank.json} | 0 test/fixtures/search0.json | 60 ++++++++++ test/fixtures/search1.json | 60 ++++++++++ test/testhelpers.go | 11 +- 9 files changed, 324 insertions(+), 117 deletions(-) create mode 100644 pkg/connector/client/path.go rename test/fixtures/{users2.json => blank.json} (100%) create mode 100644 test/fixtures/search0.json create mode 100644 test/fixtures/search1.json diff --git a/pkg/connector/client/confluence.go b/pkg/connector/client/confluence.go index 49c11360..924b1acb 100644 --- a/pkg/connector/client/confluence.go +++ b/pkg/connector/client/confluence.go @@ -20,17 +20,7 @@ import ( ) const ( - maxResults = 50 - currentUserUrlPath = "/wiki/rest/api/user/current" - GroupsListUrlPath = "/wiki/rest/api/group" - getUsersByGroupIdUrlPath = "/wiki/rest/api/group/%s/membersByGroupId" - addUsersToGroupUrlPath = "/wiki/rest/api/group/userByGroupId?groupId=%s" - removeUsersFromGroupUrlPath = "/wiki/rest/api/group/userByGroupId?groupId=%s&accountId=%s" - spacePermissionsCreateUrlPath = "/wiki/rest/api/space/%s/permissions" - spacePermissionsUpdateUrlPath = "/wiki/rest/api/space/%s/permissions/%s" - SpacesListUrlPath = "/wiki/api/v2/spaces" - spacesGetUrlPath = "/wiki/api/v2/spaces/%s" - SpacePermissionsListUrlPath = "/wiki/api/v2/spaces/%s/permissions" + maxResults = 50 ) type RequestError struct { @@ -87,7 +77,7 @@ func NewConfluenceClient(ctx context.Context, user, apiKey, domain string) (*Con } func (c *ConfluenceClient) Verify(ctx context.Context) error { - currentUserUrl, err := c.genURLNonPaginated(currentUserUrlPath) + currentUserUrl, err := c.genURLNonPaginated(CurrentUserUrlPath) if err != nil { return err } @@ -389,3 +379,47 @@ func isRatelimited( statusCode, ) } + +// GetUsersFromSearch There are no official, documented ways to get lists of +// users in Confluence. One way to get users is to issue a CQL search query with +// no conditions. The documentation mentions that queries return "up to 10k" +// users. So that may end up being a limitation of this approach. +func (c *ConfluenceClient) GetUsersFromSearch( + ctx context.Context, + pageToken string, + pageSize int, +) ( + []ConfluenceUser, + string, + *v2.RateLimitDescription, + error, +) { + getUsersUrl, err := c.parse( + SearchUrlPath, + withLimitAndOffset(pageToken, pageSize), + ) + if err != nil { + return nil, "", nil, err + } + + var response *ConfluenceSearchList + ratelimitData, err := c.get(ctx, getUsersUrl, &response) + if err != nil { + return nil, "", ratelimitData, err + } + + users := make([]ConfluenceUser, 0) + for _, user := range response.Results { + users = append(users, user.User) + } + + // The only way we can tell that we've hit the end of the list is if we get + // back fewer results than we asked for. If we get the last page but there + // are `pageSize`, then `.List()` still has to fetch the blank next page. + if len(users) < pageSize { + return users, "", ratelimitData, nil + } + + token := incToken(pageToken, len(users)) + return users, token, ratelimitData, nil +} diff --git a/pkg/connector/client/models.go b/pkg/connector/client/models.go index d55f8777..64c9d607 100644 --- a/pkg/connector/client/models.go +++ b/pkg/connector/client/models.go @@ -9,7 +9,7 @@ type ConfluenceUser struct { AccountId string `json:"accountId"` AccountType string `json:"accountType"` DisplayName string `json:"displayName"` - Email string `json:"email"` + Email string `json:"email,omitempty"` } type confluenceUserList struct { @@ -20,6 +20,21 @@ type confluenceUserList struct { Results []ConfluenceUser `json:"results"` } +type ConfluenceSearch struct { + EntityType string `json:"entityType"` + Score float64 `json:"score"` + Title string `json:"title"` + User ConfluenceUser `json:"user"` +} + +type ConfluenceSearchList struct { + Start int `json:"start"` + Limit int `json:"limit"` + TotalSize int `json:"totalSize"` + Size int `json:"size"` + Results []ConfluenceSearch `json:"results"` +} + type ConfluenceGroup struct { Type string Name string diff --git a/pkg/connector/client/path.go b/pkg/connector/client/path.go new file mode 100644 index 00000000..d9c7edff --- /dev/null +++ b/pkg/connector/client/path.go @@ -0,0 +1,102 @@ +package client + +import ( + "fmt" + "net/url" + "strconv" +) + +const ( + CurrentUserUrlPath = "/wiki/rest/api/user/current" + GroupsListUrlPath = "/wiki/rest/api/group" + getUsersByGroupIdUrlPath = "/wiki/rest/api/group/%s/membersByGroupId" + groupBaseUrlPath = "/wiki/rest/api/group/userByGroupId" + spacePermissionsCreateUrlPath = "/wiki/rest/api/space/%s/permissions" + spacePermissionsUpdateUrlPath = "/wiki/rest/api/space/%s/permissions/%s" + SpacesListUrlPath = "/wiki/api/v2/spaces" + spacesGetUrlPath = "/wiki/api/v2/spaces/%s" + SpacePermissionsListUrlPath = "/wiki/api/v2/spaces/%s/permissions" + SearchUrlPath = "/wiki/rest/api/search/user" + addUsersToGroupUrlPath = "/wiki/rest/api/group/userByGroupId?groupId=%s" + removeUsersFromGroupUrlPath = "/wiki/rest/api/group/userByGroupId?groupId=%s&accountId=%s" +) + +type Option = func(*url.URL) (*url.URL, error) + +func withQueryParameters(parameters map[string]interface{}) Option { + return func(url *url.URL) (*url.URL, error) { + query := url.Query() + for key, interfaceValue := range parameters { + var stringValue string + switch actualValue := interfaceValue.(type) { + case string: + stringValue = actualValue + case int: + stringValue = strconv.Itoa(actualValue) + case bool: + if actualValue { + stringValue = "1" + } else { + stringValue = "0" + } + default: + return nil, fmt.Errorf("invalid query parameter type %s", actualValue) + } + query.Set(key, stringValue) + } + url.RawQuery = query.Encode() + return url, nil + } +} + +// withLimitAndOffset adds `start` and `limit` query parameters to a URL. This +// pagination parameter is only used by the v1 REST API. +func withLimitAndOffset(pageToken string, pageSize int) Option { + maximum := pageSize + if maximum == 0 || maximum > maxResults { + maximum = maxResults + } + + return withQueryParameters(map[string]interface{}{ + "limit": maximum, + "start": pageToken, + }) +} + +// withPaginationCursor uses Confluence Cloud's REST API v2 pagination scheme. +func withPaginationCursor( + pageSize int, + paginationCursor string, +) Option { + maximum := pageSize + if maximum == 0 || maximum > maxResults { + maximum = maxResults + } + + parameters := map[string]interface{}{ + "limit": maximum, + } + if paginationCursor != "" { + parameters["cursor"] = paginationCursor + } + + return withQueryParameters(parameters) +} + +func (c *ConfluenceClient) parse( + path string, + options ...Option, +) (*url.URL, error) { + parsed, err := url.Parse(path) + if err != nil { + return nil, fmt.Errorf("failed to parse request path '%s': %w", path, err) + } + parsedUrl := c.apiBase.ResolveReference(parsed) + for _, option := range options { + parsedUrl, err = option(parsedUrl) + if err != nil { + return nil, err + } + } + return parsedUrl, nil +} diff --git a/pkg/connector/user.go b/pkg/connector/user.go index e07ecca1..e1faf12c 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -2,7 +2,6 @@ package connector import ( "context" - "fmt" "github.com/conductorone/baton-confluence/pkg/connector/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" @@ -52,17 +51,18 @@ func userResource(ctx context.Context, user *client.ConfluenceUser) (*v2.Resourc // parsePageToken given a marshalled pageToken as a string, return the pageToken // bag and the current page number. func parsePageToken( - pageToken string, + pToken *pagination.Token, resourceID *v2.ResourceId, ) ( *pagination.Bag, string, + int, error, ) { b := &pagination.Bag{} - err := b.Unmarshal(pageToken) + err := b.Unmarshal(pToken.Token) if err != nil { - return nil, "0", err + return nil, "0", 0, err } if b.Current() == nil { @@ -73,7 +73,11 @@ func parsePageToken( } page := b.PageToken() - return b, page, nil + size := pToken.Size + if size == 0 { + size = ResourcesPageSize + } + return b, page, size, nil } func (o *userResourceType) List( @@ -89,117 +93,42 @@ func (o *userResourceType) List( logger := ctxzap.Extract(ctx) logger.Debug("Starting Users List", zap.String("token", pToken.Token)) - // There is no Confluence Cloud REST API to get all users, so get all groups - // and then all members of each group. - - // The second parameter here is "user", which acts as a default value. - bag, page, err := parsePageToken( - pToken.Token, + bag, page, size, err := parsePageToken( + pToken, &v2.ResourceId{ResourceType: resourceTypeUser.Id}, ) + + users, nextToken, ratelimitData, err := o.client.GetUsersFromSearch(ctx, page, size) + outputAnnotations := WithRateLimitAnnotations(ratelimitData) if err != nil { - return nil, "", nil, err + return nil, "", outputAnnotations, err } - - outputResources := make([]*v2.Resource, 0) - var outputAnnotations annotations.Annotations - switch bag.ResourceTypeID() { - case resourceTypeUser.Id: - logger.Debug("Got a user from the bag", zap.String("page", page)) - if page == "" { - page = "0" - } - // Add a new page of groups - groups, nextToken, ratelimitData, err := o.client.GetGroups( - ctx, - page, - ResourcesPageSize, - ) - logger.Debug( - "Got groups", - zap.Int("len", len(groups)), - zap.String("nextToken", nextToken), - ) - outputAnnotations = WithRateLimitAnnotations(ratelimitData) - if err != nil { - return nil, "", outputAnnotations, err + rv := make([]*v2.Resource, 0) + for _, user := range users { + if user.AccountType != accountTypeAtlassian { + logger.Debug("confluence: user is not of type atlassian", zap.Any("user", user)) + continue } - // Push next page to stack. (Short-circuits if token is "".) - err = bag.Next(nextToken) + userCopy := user + ur, err := userResource(ctx, &userCopy) if err != nil { - return nil, "", outputAnnotations, err - } - - for _, group := range groups { - logger.Debug( - "adding a group to the bag", - zap.String("id", group.Id), - zap.String("name", group.Name), - ) - bag.Push( - pagination.PageState{ - ResourceTypeID: resourceTypeGroup.Id, - ResourceID: group.Id, - }, - ) + return nil, "", nil, err } - case resourceTypeGroup.Id: - currentState := bag.Current() - start := currentState.Token - if start == "" { - start = "0" - } - logger.Debug( - "Got a group from the bag", - zap.String("start", start), - zap.String("group_id", currentState.ResourceID), - ) - - // Get users for this group. - users, nextToken, ratelimitData, err := o.client.GetGroupMembers( - ctx, - start, - ResourcesPageSize, - currentState.ResourceID, - ) - outputAnnotations = WithRateLimitAnnotations(ratelimitData) - if err != nil { - return nil, "", outputAnnotations, err - } - - // Push next page to stack. (Short-circuits if token is "".) - err = bag.Next(nextToken) - if err != nil { - return nil, "", outputAnnotations, err - } - - // Add users to output resources. There will be duplicates across groups. - for _, user := range users { - if user.AccountType != accountTypeAtlassian { - logger.Debug("confluence: user is not of type atlassian", zap.Any("user", user)) - continue - } - - userCopy := user - newUserResource, err := userResource(ctx, &userCopy) - if err != nil { - return nil, "", nil, err - } - - outputResources = append(outputResources, newUserResource) - } - default: - return nil, "", nil, fmt.Errorf("unexpected resource type while fetching list of users") + rv = append(rv, ur) + } + err = bag.Next(nextToken) + if err != nil { + return nil, "", nil, err } - pageToken, err := bag.Marshal() + nextToken, err = bag.Marshal() if err != nil { return nil, "", nil, err } - return outputResources, pageToken, outputAnnotations, nil + return rv, nextToken, outputAnnotations, nil } func (o *userResourceType) Entitlements( diff --git a/pkg/connector/user_test.go b/pkg/connector/user_test.go index b40e7043..428e650d 100644 --- a/pkg/connector/user_test.go +++ b/pkg/connector/user_test.go @@ -32,7 +32,7 @@ func TestUsersList(t *testing.T) { resources := make([]*v2.Resource, 0) bag := &pagination.Bag{} for { - pToken := pagination.Token{} + pToken := pagination.Token{Size: 2} state := bag.Current() if state != nil { token, _ := bag.Marshal() diff --git a/test/fixtures/users2.json b/test/fixtures/blank.json similarity index 100% rename from test/fixtures/users2.json rename to test/fixtures/blank.json diff --git a/test/fixtures/search0.json b/test/fixtures/search0.json new file mode 100644 index 00000000..d12eb60a --- /dev/null +++ b/test/fixtures/search0.json @@ -0,0 +1,60 @@ +{ + "results": [ + { + "user": { + "type": "known", + "accountId": "234", + "accountType": "atlassian", + "email": "marcos.gaeta@conductorone.com", + "publicName": "Marcos Gaeta", + "profilePicture": { + "path": "/wiki/aa-avatar/234", + "width": 48, + "height": 48, + "isDefault": false + }, + "displayName": "Marcos Gaeta", + "isExternalCollaborator": false, + "_expandable": { + "operations": "", + "personalSpace": "" + }, + "_links": { + "self": "https://conductorone.atlassian.net/wiki/rest/api/user?accountId=234" + } + } + }, + { + "user": { + "type": "known", + "accountId": "345", + "accountType": "app", + "email": "robot@conductorone.com", + "publicName": "Robot", + "profilePicture": { + "path": "/wiki/aa-avatar/345", + "width": 48, + "height": 48, + "isDefault": false + }, + "displayName": "Robot", + "isExternalCollaborator": false, + "_expandable": { + "operations": "", + "personalSpace": "" + }, + "_links": { + "self": "https://conductorone.atlassian.net/wiki/rest/api/user?accountId=345" + } + } + } + ], + "start": 0, + "limit": 200, + "size": 2, + "_links": { + "base": "https://conductorone.atlassian.net/wiki", + "context": "/wiki", + "self": "https://conductorone.atlassian.net/wiki/rest/api/group/123/membersByGroupId" + } +} \ No newline at end of file diff --git a/test/fixtures/search1.json b/test/fixtures/search1.json new file mode 100644 index 00000000..c0f88490 --- /dev/null +++ b/test/fixtures/search1.json @@ -0,0 +1,60 @@ +{ + "results": [ + { + "user": { + "type": "known", + "accountId": "234", + "accountType": "atlassian", + "email": "marcos.gaeta@conductorone.com", + "publicName": "Marcos Gaeta", + "profilePicture": { + "path": "/wiki/aa-avatar/234", + "width": 48, + "height": 48, + "isDefault": false + }, + "displayName": "Marcos Gaeta", + "isExternalCollaborator": false, + "_expandable": { + "operations": "", + "personalSpace": "" + }, + "_links": { + "self": "https://conductorone.atlassian.net/wiki/rest/api/user?accountId=234" + } + } + }, + { + "user": { + "type": "known", + "accountId": "789", + "accountType": "atlassian", + "email": "other.user@conductorone.com", + "publicName": "Other User", + "profilePicture": { + "path": "/wiki/aa-avatar/789", + "width": 48, + "height": 48, + "isDefault": false + }, + "displayName": "Other User", + "isExternalCollaborator": false, + "_expandable": { + "operations": "", + "personalSpace": "" + }, + "_links": { + "self": "https://conductorone.atlassian.net/wiki/rest/api/user?accountId=789" + } + } + } + ], + "start": 0, + "limit": 200, + "size": 2, + "_links": { + "base": "https://conductorone.atlassian.net/wiki", + "context": "/wiki", + "self": "https://conductorone.atlassian.net/wiki/rest/api/group/456/membersByGroupId" + } +} \ No newline at end of file diff --git a/test/testhelpers.go b/test/testhelpers.go index 3ef0520e..842de58e 100644 --- a/test/testhelpers.go +++ b/test/testhelpers.go @@ -1,6 +1,7 @@ package test import ( + "fmt" "net/http" "net/http/httptest" "os" @@ -49,8 +50,9 @@ func FixturesServer() *httptest.Server { var filename string routeUrl := request.URL.String() switch { - case strings.Contains(routeUrl, "group/member") && strings.Contains(routeUrl, "start=2"): - filename = "../../test/fixtures/users2.json" + case strings.Contains(routeUrl, "group/member") && strings.Contains(routeUrl, "start=2") || + (strings.Contains(routeUrl, client.SearchUrlPath) && strings.Contains(routeUrl, "start=4")): + filename = "../../test/fixtures/blank.json" case (strings.Contains(routeUrl, "group/member") && strings.Contains(routeUrl, "confluence-users")) || (strings.Contains(routeUrl, client.GroupsListUrlPath) && strings.Contains(routeUrl, "123")): filename = "../../test/fixtures/users0.json" @@ -67,8 +69,13 @@ func FixturesServer() *httptest.Server { filename = "../../test/fixtures/spaces1.json" case strings.Contains(routeUrl, client.SpacesListUrlPath) && !strings.Contains(routeUrl, "cursor"): filename = "../../test/fixtures/spaces0.json" + case strings.Contains(routeUrl, client.SearchUrlPath) && strings.Contains(routeUrl, "start=0"): + filename = "../../test/fixtures/search0.json" + case strings.Contains(routeUrl, client.SearchUrlPath) && strings.Contains(routeUrl, "start=2"): + filename = "../../test/fixtures/search1.json" default: // This should never happen in tests. + panic(fmt.Errorf("bad url: %s", routeUrl)) return } data, _ := os.ReadFile(filename) From a22e7a453da8cd5e046216fd203a6f8724e2114f Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Tue, 30 Jul 2024 12:29:20 -0700 Subject: [PATCH 06/13] lint --- pkg/connector/client/path.go | 4 ++-- pkg/connector/user.go | 3 +++ test/testhelpers.go | 1 - 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/connector/client/path.go b/pkg/connector/client/path.go index d9c7edff..6f0bb1a1 100644 --- a/pkg/connector/client/path.go +++ b/pkg/connector/client/path.go @@ -63,8 +63,8 @@ func withLimitAndOffset(pageToken string, pageSize int) Option { }) } -// withPaginationCursor uses Confluence Cloud's REST API v2 pagination scheme. -func withPaginationCursor( +// WithPaginationCursor uses Confluence Cloud's REST API v2 pagination scheme. +func WithPaginationCursor( pageSize int, paginationCursor string, ) Option { diff --git a/pkg/connector/user.go b/pkg/connector/user.go index e1faf12c..e669eacb 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -97,6 +97,9 @@ func (o *userResourceType) List( pToken, &v2.ResourceId{ResourceType: resourceTypeUser.Id}, ) + if err != nil { + return nil, "", nil, err + } users, nextToken, ratelimitData, err := o.client.GetUsersFromSearch(ctx, page, size) outputAnnotations := WithRateLimitAnnotations(ratelimitData) diff --git a/test/testhelpers.go b/test/testhelpers.go index 842de58e..d3c68363 100644 --- a/test/testhelpers.go +++ b/test/testhelpers.go @@ -76,7 +76,6 @@ func FixturesServer() *httptest.Server { default: // This should never happen in tests. panic(fmt.Errorf("bad url: %s", routeUrl)) - return } data, _ := os.ReadFile(filename) _, err := writer.Write(data) From cc9188ac107505c989a6a08a3ad8891175542a71 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Tue, 30 Jul 2024 12:45:37 -0700 Subject: [PATCH 07/13] fix tests --- pkg/connector/user.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/connector/user.go b/pkg/connector/user.go index e669eacb..f92ef158 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -77,6 +77,9 @@ func parsePageToken( if size == 0 { size = ResourcesPageSize } + if page == "" { + page = "0" + } return b, page, size, nil } From 0e558ab9c09220c9087b66f143f115ef2935a474 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Tue, 30 Jul 2024 13:32:19 -0700 Subject: [PATCH 08/13] fix missing query parameter --- pkg/connector/client/confluence.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/connector/client/confluence.go b/pkg/connector/client/confluence.go index 924b1acb..def369d9 100644 --- a/pkg/connector/client/confluence.go +++ b/pkg/connector/client/confluence.go @@ -397,6 +397,9 @@ func (c *ConfluenceClient) GetUsersFromSearch( getUsersUrl, err := c.parse( SearchUrlPath, withLimitAndOffset(pageToken, pageSize), + withQueryParameters(map[string]interface{}{ + "cql": "type=user", + }), ) if err != nil { return nil, "", nil, err From e708c31791d0ea821caebe0d6cc06c5ab3c0aa9d Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Tue, 30 Jul 2024 15:15:03 -0700 Subject: [PATCH 09/13] filter out deactivated users --- pkg/connector/client/confluence.go | 8 ++++++- pkg/connector/client/models.go | 14 +++++++---- pkg/connector/group.go | 6 +---- pkg/connector/helpers.go | 19 +++++++++++++++ pkg/connector/user.go | 3 +-- pkg/connector/user_test.go | 2 +- test/fixtures/search0.json | 38 ++++++++++++++++++++++++++++-- test/fixtures/search1.json | 14 +++++++++-- test/fixtures/users0.json | 14 +++++++++-- test/fixtures/users1.json | 14 +++++++++-- test/testhelpers.go | 4 ++-- 11 files changed, 113 insertions(+), 23 deletions(-) diff --git a/pkg/connector/client/confluence.go b/pkg/connector/client/confluence.go index def369d9..6c59af1a 100644 --- a/pkg/connector/client/confluence.go +++ b/pkg/connector/client/confluence.go @@ -143,7 +143,13 @@ func (c *ConfluenceClient) GetGroupMembers( *v2.RateLimitDescription, error, ) { - getUsersUrl, err := c.genURL(pageToken, pageSize, fmt.Sprintf(getUsersByGroupIdUrlPath, groupId)) + getUsersUrl, err := c.parse( + fmt.Sprintf(getUsersByGroupIdUrlPath, groupId), + withLimitAndOffset(pageToken, pageSize), + withQueryParameters(map[string]interface{}{ + "expand": "operations", + }), + ) if err != nil { return nil, "", nil, err } diff --git a/pkg/connector/client/models.go b/pkg/connector/client/models.go index 64c9d607..a14d8279 100644 --- a/pkg/connector/client/models.go +++ b/pkg/connector/client/models.go @@ -6,10 +6,16 @@ type ConfluenceLink struct { } type ConfluenceUser struct { - AccountId string `json:"accountId"` - AccountType string `json:"accountType"` - DisplayName string `json:"displayName"` - Email string `json:"email,omitempty"` + AccountId string `json:"accountId"` + AccountType string `json:"accountType"` + DisplayName string `json:"displayName"` + Email string `json:"email,omitempty"` + Operations []ConfluenceOperation `json:"operations,omitempty"` +} + +type ConfluenceOperation struct { + Operation string `json:"operation"` + TargetType string `json:"targetType"` } type confluenceUserList struct { diff --git a/pkg/connector/group.go b/pkg/connector/group.go index ca8e5d09..901d67c1 100644 --- a/pkg/connector/group.go +++ b/pkg/connector/group.go @@ -11,8 +11,6 @@ import ( "github.com/conductorone/baton-sdk/pkg/types/entitlement" "github.com/conductorone/baton-sdk/pkg/types/grant" "github.com/conductorone/baton-sdk/pkg/types/resource" - "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" - "go.uber.org/zap" ) const ( @@ -133,7 +131,6 @@ func (o *groupResourceType) Grants( annotations.Annotations, error, ) { - l := ctxzap.Extract(ctx) bag := &pagination.Bag{} err := bag.Unmarshal(pt.Token) if err != nil { @@ -158,8 +155,7 @@ func (o *groupResourceType) Grants( var rv []*v2.Grant for _, user := range users { - if user.AccountType != accountTypeAtlassian { - l.Debug("confluence: user is not of type atlassian", zap.Any("user", user)) + if !shouldIncludeUser(ctx, user) { continue } diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index 7991af83..c058d6cd 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -1,8 +1,13 @@ package connector import ( + "context" + + "github.com/conductorone/baton-confluence/pkg/connector/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" ) const ResourcesPageSize = 100 @@ -23,3 +28,17 @@ func WithRateLimitAnnotations( return outputAnnotations } + +// shouldIncludeUser only include extant, human users. +func shouldIncludeUser(ctx context.Context, user client.ConfluenceUser) bool { + logger := ctxzap.Extract(ctx) + if user.AccountType != accountTypeAtlassian { + logger.Debug("confluence: user is not of type atlassian", zap.Any("user", user)) + return false + } + if len(user.Operations) == 0 { + logger.Debug("confluence: user is deactivated (Unlicensed)", zap.Any("user", user)) + return false + } + return true +} diff --git a/pkg/connector/user.go b/pkg/connector/user.go index f92ef158..e0a18f14 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -111,8 +111,7 @@ func (o *userResourceType) List( } rv := make([]*v2.Resource, 0) for _, user := range users { - if user.AccountType != accountTypeAtlassian { - logger.Debug("confluence: user is not of type atlassian", zap.Any("user", user)) + if !shouldIncludeUser(ctx, user) { continue } diff --git a/pkg/connector/user_test.go b/pkg/connector/user_test.go index 428e650d..2fa8d272 100644 --- a/pkg/connector/user_test.go +++ b/pkg/connector/user_test.go @@ -14,7 +14,7 @@ import ( func TestUsersList(t *testing.T) { ctx := context.Background() - t.Run("should get users, using pagination, ignoring robots", func(t *testing.T) { + t.Run("should get users, using pagination, ignoring robots & deactivated", func(t *testing.T) { server := test.FixturesServer() defer server.Close() diff --git a/test/fixtures/search0.json b/test/fixtures/search0.json index d12eb60a..06fe736e 100644 --- a/test/fixtures/search0.json +++ b/test/fixtures/search0.json @@ -15,8 +15,13 @@ }, "displayName": "Marcos Gaeta", "isExternalCollaborator": false, + "operations": [ + { + "targetType": "application", + "operation": "use" + } + ], "_expandable": { - "operations": "", "personalSpace": "" }, "_links": { @@ -39,14 +44,43 @@ }, "displayName": "Robot", "isExternalCollaborator": false, + "operations": [ + { + "targetType": "application", + "operation": "use" + } + ], "_expandable": { - "operations": "", "personalSpace": "" }, "_links": { "self": "https://conductorone.atlassian.net/wiki/rest/api/user?accountId=345" } } + }, + { + "user": { + "type": "known", + "accountId": "321", + "accountType": "atlassian", + "email": "deactivated@conductorone.com", + "publicName": "Deactivated (Unlicensed)", + "profilePicture": { + "path": "/wiki/aa-avatar/321", + "width": 48, + "height": 48, + "isDefault": false + }, + "displayName": "Deactivated", + "isExternalCollaborator": false, + "operations": [], + "_expandable": { + "personalSpace": "" + }, + "_links": { + "self": "https://conductorone.atlassian.net/wiki/rest/api/user?accountId=321" + } + } } ], "start": 0, diff --git a/test/fixtures/search1.json b/test/fixtures/search1.json index c0f88490..ae4ce643 100644 --- a/test/fixtures/search1.json +++ b/test/fixtures/search1.json @@ -15,8 +15,13 @@ }, "displayName": "Marcos Gaeta", "isExternalCollaborator": false, + "operations": [ + { + "targetType": "application", + "operation": "use" + } + ], "_expandable": { - "operations": "", "personalSpace": "" }, "_links": { @@ -39,8 +44,13 @@ }, "displayName": "Other User", "isExternalCollaborator": false, + "operations": [ + { + "targetType": "application", + "operation": "use" + } + ], "_expandable": { - "operations": "", "personalSpace": "" }, "_links": { diff --git a/test/fixtures/users0.json b/test/fixtures/users0.json index b73b73b7..b2409cab 100644 --- a/test/fixtures/users0.json +++ b/test/fixtures/users0.json @@ -14,8 +14,13 @@ }, "displayName": "Marcos Gaeta", "isExternalCollaborator": false, + "operations": [ + { + "targetType": "application", + "operation": "use" + } + ], "_expandable": { - "operations": "", "personalSpace": "" }, "_links": { @@ -36,8 +41,13 @@ }, "displayName": "Robot", "isExternalCollaborator": false, + "operations": [ + { + "targetType": "application", + "operation": "use" + } + ], "_expandable": { - "operations": "", "personalSpace": "" }, "_links": { diff --git a/test/fixtures/users1.json b/test/fixtures/users1.json index 673ae281..3b35158e 100644 --- a/test/fixtures/users1.json +++ b/test/fixtures/users1.json @@ -14,8 +14,13 @@ }, "displayName": "Marcos Gaeta", "isExternalCollaborator": false, + "operations": [ + { + "targetType": "application", + "operation": "use" + } + ], "_expandable": { - "operations": "", "personalSpace": "" }, "_links": { @@ -36,8 +41,13 @@ }, "displayName": "Other User", "isExternalCollaborator": false, + "operations": [ + { + "targetType": "application", + "operation": "use" + } + ], "_expandable": { - "operations": "", "personalSpace": "" }, "_links": { diff --git a/test/testhelpers.go b/test/testhelpers.go index d3c68363..784591cb 100644 --- a/test/testhelpers.go +++ b/test/testhelpers.go @@ -51,7 +51,7 @@ func FixturesServer() *httptest.Server { routeUrl := request.URL.String() switch { case strings.Contains(routeUrl, "group/member") && strings.Contains(routeUrl, "start=2") || - (strings.Contains(routeUrl, client.SearchUrlPath) && strings.Contains(routeUrl, "start=4")): + (strings.Contains(routeUrl, client.SearchUrlPath) && strings.Contains(routeUrl, "start=5")): filename = "../../test/fixtures/blank.json" case (strings.Contains(routeUrl, "group/member") && strings.Contains(routeUrl, "confluence-users")) || (strings.Contains(routeUrl, client.GroupsListUrlPath) && strings.Contains(routeUrl, "123")): @@ -71,7 +71,7 @@ func FixturesServer() *httptest.Server { filename = "../../test/fixtures/spaces0.json" case strings.Contains(routeUrl, client.SearchUrlPath) && strings.Contains(routeUrl, "start=0"): filename = "../../test/fixtures/search0.json" - case strings.Contains(routeUrl, client.SearchUrlPath) && strings.Contains(routeUrl, "start=2"): + case strings.Contains(routeUrl, client.SearchUrlPath) && strings.Contains(routeUrl, "start=3"): filename = "../../test/fixtures/search1.json" default: // This should never happen in tests. From 279c9eb1d47be69add69f16919c630f8c07ea27f Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Wed, 31 Jul 2024 12:08:29 -0700 Subject: [PATCH 10/13] fix --- pkg/connector/client/confluence.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/connector/client/confluence.go b/pkg/connector/client/confluence.go index 6c59af1a..79efd3b9 100644 --- a/pkg/connector/client/confluence.go +++ b/pkg/connector/client/confluence.go @@ -404,7 +404,8 @@ func (c *ConfluenceClient) GetUsersFromSearch( SearchUrlPath, withLimitAndOffset(pageToken, pageSize), withQueryParameters(map[string]interface{}{ - "cql": "type=user", + "cql": "type=user", + "expand": "operations", }), ) if err != nil { From c2a64f9a6a0f8b6ccb156737db4fbb5a49f4a42d Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Wed, 31 Jul 2024 12:40:07 -0700 Subject: [PATCH 11/13] revert user list back to 2d scheme --- pkg/connector/client/path.go | 14 +--- pkg/connector/user.go | 138 +++++++++++++++++++++++++++++------ 2 files changed, 117 insertions(+), 35 deletions(-) diff --git a/pkg/connector/client/path.go b/pkg/connector/client/path.go index 6f0bb1a1..f8ef2f74 100644 --- a/pkg/connector/client/path.go +++ b/pkg/connector/client/path.go @@ -52,13 +52,8 @@ func withQueryParameters(parameters map[string]interface{}) Option { // withLimitAndOffset adds `start` and `limit` query parameters to a URL. This // pagination parameter is only used by the v1 REST API. func withLimitAndOffset(pageToken string, pageSize int) Option { - maximum := pageSize - if maximum == 0 || maximum > maxResults { - maximum = maxResults - } - return withQueryParameters(map[string]interface{}{ - "limit": maximum, + "limit": pageSize, "start": pageToken, }) } @@ -68,13 +63,8 @@ func WithPaginationCursor( pageSize int, paginationCursor string, ) Option { - maximum := pageSize - if maximum == 0 || maximum > maxResults { - maximum = maxResults - } - parameters := map[string]interface{}{ - "limit": maximum, + "limit": pageSize, } if paginationCursor != "" { parameters["cursor"] = paginationCursor diff --git a/pkg/connector/user.go b/pkg/connector/user.go index e0a18f14..bd8b391a 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -2,6 +2,7 @@ package connector import ( "context" + "fmt" "github.com/conductorone/baton-confluence/pkg/connector/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" @@ -12,11 +13,24 @@ import ( "go.uber.org/zap" ) +const ( + GroupPageSizeMaximum = 25 +) + type userResourceType struct { resourceType *v2.ResourceType client *client.ConfluenceClient } +// limitPageSizeForGroups - Enforcing an arbitrarily small page size limit for +// groups so that we can handle the 2D pagination scheme with a comfortable margin. +func limitPageSizeForGroups(pageSize int) int { + if pageSize <= 0 || pageSize > GroupPageSizeMaximum { + return GroupPageSizeMaximum + } + return pageSize +} + func (o *userResourceType) ResourceType(_ context.Context) *v2.ResourceType { return o.resourceType } @@ -66,10 +80,12 @@ func parsePageToken( } if b.Current() == nil { - b.Push(pagination.PageState{ - ResourceTypeID: resourceID.ResourceType, - ResourceID: resourceID.Resource, - }) + b.Push( + pagination.PageState{ + ResourceTypeID: resourceID.ResourceType, + ResourceID: resourceID.Resource, + }, + ) } page := b.PageToken() @@ -96,6 +112,10 @@ func (o *userResourceType) List( logger := ctxzap.Extract(ctx) logger.Debug("Starting Users List", zap.String("token", pToken.Token)) + // There is no Confluence Cloud REST API to get all users, so get all groups + // and then all members of each group. + + // The second parameter here is "user", which acts as a default value. bag, page, size, err := parsePageToken( pToken, &v2.ResourceId{ResourceType: resourceTypeUser.Id}, @@ -104,36 +124,108 @@ func (o *userResourceType) List( return nil, "", nil, err } - users, nextToken, ratelimitData, err := o.client.GetUsersFromSearch(ctx, page, size) - outputAnnotations := WithRateLimitAnnotations(ratelimitData) - if err != nil { - return nil, "", outputAnnotations, err - } - rv := make([]*v2.Resource, 0) - for _, user := range users { - if !shouldIncludeUser(ctx, user) { - continue + outputResources := make([]*v2.Resource, 0) + var outputAnnotations annotations.Annotations + switch bag.ResourceTypeID() { + case resourceTypeUser.Id: + logger.Debug("Got a user from the bag", zap.String("page", page)) + if page == "" { + page = "0" } - userCopy := user - ur, err := userResource(ctx, &userCopy) + size = limitPageSizeForGroups(size) + + // Add a new page of groups + groups, nextToken, ratelimitData, err := o.client.GetGroups( + ctx, + page, + size, + ) + logger.Debug( + "Got groups", + zap.Int("len", len(groups)), + zap.String("nextToken", nextToken), + ) + outputAnnotations = WithRateLimitAnnotations(ratelimitData) if err != nil { - return nil, "", nil, err + return nil, "", outputAnnotations, err } - rv = append(rv, ur) - } - err = bag.Next(nextToken) - if err != nil { - return nil, "", nil, err + // Push next page to stack. (Short-circuits if token is "".) + err = bag.Next(nextToken) + if err != nil { + return nil, "", outputAnnotations, err + } + + for _, group := range groups { + logger.Debug( + "adding a group to the bag", + zap.String("id", group.Id), + zap.String("name", group.Name), + ) + bag.Push( + pagination.PageState{ + ResourceTypeID: resourceTypeGroup.Id, + ResourceID: group.Id, + }, + ) + } + case resourceTypeGroup.Id: + currentState := bag.Current() + + start := currentState.Token + if start == "" { + start = "0" + } + logger.Debug( + "Got a group from the bag", + zap.String("start", start), + zap.String("group_id", currentState.ResourceID), + ) + + // Get users for this group. + users, nextToken, ratelimitData, err := o.client.GetGroupMembers( + ctx, + start, + size, + currentState.ResourceID, + ) + outputAnnotations = WithRateLimitAnnotations(ratelimitData) + if err != nil { + return nil, "", outputAnnotations, err + } + + // Push next page to stack. (Short-circuits if token is "".) + err = bag.Next(nextToken) + if err != nil { + return nil, "", outputAnnotations, err + } + + // Add users to output resources. There will be duplicates across groups. + for _, user := range users { + if user.AccountType != accountTypeAtlassian { + logger.Debug("confluence: user is not of type atlassian", zap.Any("user", user)) + continue + } + + userCopy := user + newUserResource, err := userResource(ctx, &userCopy) + if err != nil { + return nil, "", nil, err + } + + outputResources = append(outputResources, newUserResource) + } + default: + return nil, "", nil, fmt.Errorf("unexpected resource type while fetching list of users") } - nextToken, err = bag.Marshal() + pageToken, err := bag.Marshal() if err != nil { return nil, "", nil, err } - return rv, nextToken, outputAnnotations, nil + return outputResources, pageToken, outputAnnotations, nil } func (o *userResourceType) Entitlements( From 976ca0050ed301ed5ed20c3e24a0b438f33e41fa Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Thu, 1 Aug 2024 17:44:28 -0700 Subject: [PATCH 12/13] User search + group members --- pkg/connector/user.go | 53 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/pkg/connector/user.go b/pkg/connector/user.go index bd8b391a..ce682a75 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -112,14 +112,13 @@ func (o *userResourceType) List( logger := ctxzap.Extract(ctx) logger.Debug("Starting Users List", zap.String("token", pToken.Token)) - // There is no Confluence Cloud REST API to get all users, so get all groups - // and then all members of each group. + // Unfortunately, there is no Confluence Cloud REST API to get all users. We + // try to get all the users in a roundabout away by using three other APIs. + // First get all groups in Confluence and then for each group get all + // members. Finally, we use User Search to try and find any users that were + // not a part of any groups. - // The second parameter here is "user", which acts as a default value. - bag, page, size, err := parsePageToken( - pToken, - &v2.ResourceId{ResourceType: resourceTypeUser.Id}, - ) + bag, page, size, err := parsePageToken(pToken, &v2.ResourceId{}) if err != nil { return nil, "", nil, err } @@ -127,8 +126,43 @@ func (o *userResourceType) List( outputResources := make([]*v2.Resource, 0) var outputAnnotations annotations.Annotations switch bag.ResourceTypeID() { + case "": + users, nextToken, ratelimitData, err := o.client.GetUsersFromSearch(ctx, page, size) + outputAnnotations := WithRateLimitAnnotations(ratelimitData) + if err != nil { + return nil, "", outputAnnotations, err + } + for _, user := range users { + if !shouldIncludeUser(ctx, user) { + continue + } + + userCopy := user + newUserResource, err := userResource(ctx, &userCopy) + if err != nil { + return nil, "", nil, err + } + + outputResources = append(outputResources, newUserResource) + } + + err = bag.Next(nextToken) + if err != nil { + return nil, "", nil, err + } + + if bag.Current() == nil { + logger.Debug("Finished User Search, moving on to 2D query") + bag.Push( + pagination.PageState{ + // Using "user" here as a placeholder so the for loop know to start again. + ResourceTypeID: resourceTypeUser.Id, + }, + ) + } + case resourceTypeUser.Id: - logger.Debug("Got a user from the bag", zap.String("page", page)) + logger.Debug("Got the placeholder from the bag", zap.String("page", page)) if page == "" { page = "0" } @@ -203,8 +237,7 @@ func (o *userResourceType) List( // Add users to output resources. There will be duplicates across groups. for _, user := range users { - if user.AccountType != accountTypeAtlassian { - logger.Debug("confluence: user is not of type atlassian", zap.Any("user", user)) + if !shouldIncludeUser(ctx, user) { continue } From a06e2c5d0fef5008932803af8310cb52fb9f583b Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Fri, 2 Aug 2024 11:09:15 -0700 Subject: [PATCH 13/13] fix tests --- pkg/connector/user_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/connector/user_test.go b/pkg/connector/user_test.go index 2fa8d272..a095c76e 100644 --- a/pkg/connector/user_test.go +++ b/pkg/connector/user_test.go @@ -8,6 +8,7 @@ import ( "github.com/conductorone/baton-confluence/test" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/pagination" + mapset "github.com/deckarep/golang-set/v2" "github.com/stretchr/testify/require" ) @@ -55,8 +56,15 @@ func TestUsersList(t *testing.T) { } require.NotNil(t, resources) - // We expect there to be duplicates. - require.Len(t, resources, 3) + // We expect there to be duplicates from users being in multiple groups + // and then showing up in User Search. + require.Len(t, resources, 6) require.NotEmpty(t, resources[0].Id) + + allIDs := mapset.NewSet[string]() + for _, resource := range resources { + allIDs.Add(resource.Id.Resource) + } + require.Equal(t, allIDs.Cardinality(), 2) }) }