diff --git a/pkg/accessreview/drivers/google_analytics.go b/pkg/accessreview/drivers/google_analytics.go index 604a8e305..0665b2db4 100644 --- a/pkg/accessreview/drivers/google_analytics.go +++ b/pkg/accessreview/drivers/google_analytics.go @@ -23,6 +23,7 @@ package drivers import ( "context" "encoding/json" + "errors" "fmt" "net/http" "net/url" @@ -101,7 +102,7 @@ func (d *GoogleAnalyticsDriver) ListAccounts(ctx context.Context) ([]AccountReco // Account-level bindings. if err := d.collectBindings(ctx, members, "v1alpha", "accounts", url.PathEscape(d.accountID), "accessBindings"); err != nil { - return nil, err + return nil, fmt.Errorf("cannot list google analytics access bindings for account %q: %w", d.accountID, err) } // Property-level bindings, one loop per property beneath the account. @@ -111,9 +112,21 @@ func (d *GoogleAnalyticsDriver) ListAccounts(ctx context.Context) ([]AccountReco } for _, propertyID := range propertyIDs { - if err := d.collectBindings(ctx, members, "v1alpha", "properties", url.PathEscape(propertyID), "accessBindings"); err != nil { - return nil, err + err := d.collectBindings(ctx, members, "v1alpha", "properties", url.PathEscape(propertyID), "accessBindings") + if err == nil { + continue } + + // A property the token cannot read, or one deleted between the list + // and the read, must not discard the bindings already collected: an + // account whose properties are 49/50 readable is still worth + // reviewing. Anything else invalidates the whole fetch. + if e, ok := errors.AsType[*googleAnalyticsStatusError](err); ok && + (e.status == http.StatusForbidden || e.status == http.StatusNotFound) { + continue + } + + return nil, fmt.Errorf("cannot list google analytics access bindings for property %q: %w", propertyID, err) } return googleAnalyticsRecords(members), nil @@ -187,6 +200,17 @@ func (d *GoogleAnalyticsDriver) listProperties(ctx context.Context) ([]string, e return nil, fmt.Errorf("cannot list all google analytics properties: %w", ErrPaginationLimitReached) } +// googleAnalyticsStatusError carries the HTTP status of a failed Admin API call +// so callers can tell a per-resource permission problem apart from a failure +// that invalidates the whole fetch. +type googleAnalyticsStatusError struct { + status int +} + +func (e *googleAnalyticsStatusError) Error() string { + return fmt.Sprintf("unexpected status %d", e.status) +} + func (d *GoogleAnalyticsDriver) getJSON(ctx context.Context, endpoint string, out any) error { req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { @@ -205,7 +229,7 @@ func (d *GoogleAnalyticsDriver) getJSON(ctx context.Context, endpoint string, ou }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return fmt.Errorf("cannot fetch google analytics resource: unexpected status %d", httpResp.StatusCode) + return &googleAnalyticsStatusError{status: httpResp.StatusCode} } if err := json.NewDecoder(httpResp.Body).Decode(out); err != nil { @@ -217,6 +241,7 @@ func (d *GoogleAnalyticsDriver) getJSON(ctx context.Context, endpoint string, ou // googleAnalyticsURL builds a v1alpha Admin API URL from path segments, adding // the shared pageSize, an optional page token, and any extra query values. +// Keys present in extra replace the default rather than adding to it. func googleAnalyticsURL(pageToken string, extra url.Values, segments ...string) (string, error) { joined, err := url.JoinPath("https://"+googleAnalyticsAPIHost, segments...) if err != nil { @@ -232,6 +257,8 @@ func googleAnalyticsURL(pageToken string, extra url.Values, segments ...string) q.Set("pageSize", strconv.Itoa(googleAnalyticsPageSize)) for k, vs := range extra { + q.Del(k) + for _, v := range vs { q.Add(k, v) } @@ -246,6 +273,19 @@ func googleAnalyticsURL(pageToken string, extra url.Values, segments ...string) return parsed.String(), nil } +// GoogleAnalyticsAccountBindingsProbeURL builds a single-item account-level +// accessBindings request for accountID. The connection probe uses it so the +// check exercises the permission the driver actually needs — Administrator on +// the account, granted through analytics.manage.users.readonly — instead of the +// accounts list, which any analytics.readonly grant can call. +func GoogleAnalyticsAccountBindingsProbeURL(accountID string) (string, error) { + return googleAnalyticsURL( + "", + url.Values{"pageSize": {"1"}}, + "v1alpha", "accounts", url.PathEscape(accountID), "accessBindings", + ) +} + // addGoogleAnalyticsBinding folds one access binding into the per-email member // map, deduplicating roles and setting the admin flag when the admin role is // present. diff --git a/pkg/accessreview/drivers/google_analytics_test.go b/pkg/accessreview/drivers/google_analytics_test.go index e2dc819a1..36dd9dbc4 100644 --- a/pkg/accessreview/drivers/google_analytics_test.go +++ b/pkg/accessreview/drivers/google_analytics_test.go @@ -38,7 +38,7 @@ func TestGoogleAnalyticsDriver(t *testing.T) { driver := NewGoogleAnalyticsDriver(client, "123456") records, err := driver.ListAccounts(context.Background()) require.NoError(t, err) - require.Len(t, records, 3) + require.Len(t, records, 4) // alice holds an account-level admin binding AND a property-level viewer // binding: roles are merged, deduplicated, sorted, prefix-stripped, and the @@ -62,4 +62,14 @@ func TestGoogleAnalyticsDriver(t *testing.T) { assert.Equal(t, "carol@example.com", carol.Email) assert.False(t, carol.IsAdmin) assert.Equal(t, []string{"analyst"}, carol.Roles) + + // dave holds a binding only on properties/99999, a subproperty parented to + // another property rather than to the account. He is reachable only through + // the ancestor filter, so his presence is what proves subproperties are + // walked. A third property (55555) returns 403 and is skipped rather than + // failing the whole account — otherwise these four records would be zero. + dave := records[3] + assert.Equal(t, "dave@example.com", dave.Email) + assert.False(t, dave.IsAdmin) + assert.Equal(t, []string{"analyst"}, dave.Roles) } diff --git a/pkg/accessreview/drivers/organizations.go b/pkg/accessreview/drivers/organizations.go index bf96c985f..b7f8b2560 100644 --- a/pkg/accessreview/drivers/organizations.go +++ b/pkg/accessreview/drivers/organizations.go @@ -25,7 +25,6 @@ import ( "encoding/json" "fmt" "net/http" - "net/url" "strconv" "strings" ) @@ -482,21 +481,12 @@ func ListGoogleAnalyticsOrganizations(ctx context.Context, httpClient *http.Clie pageToken := "" for range maxPaginationPages { - q := url.Values{} - q.Set("pageSize", strconv.Itoa(googleAnalyticsPageSize)) - - if pageToken != "" { - q.Set("pageToken", pageToken) + endpoint, err := googleAnalyticsURL(pageToken, nil, "v1alpha", "accounts") + if err != nil { + return nil, err } - endpoint := url.URL{ - Scheme: "https", - Host: googleAnalyticsAPIHost, - Path: "/v1alpha/accounts", - RawQuery: q.Encode(), - } - - req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint.String(), nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { return nil, fmt.Errorf("cannot create google analytics accounts request: %w", err) } @@ -535,7 +525,12 @@ func ListGoogleAnalyticsOrganizations(ctx context.Context, httpClient *http.Clie continue } - orgs = append(orgs, Organization{Slug: id, DisplayName: a.DisplayName}) + displayName := a.DisplayName + if displayName == "" { + displayName = id + } + + orgs = append(orgs, Organization{Slug: id, DisplayName: displayName}) } if out.NextPageToken == "" { diff --git a/pkg/accessreview/drivers/testdata/google_analytics.yaml b/pkg/accessreview/drivers/testdata/google_analytics.yaml index 744e9fd32..c3695b702 100644 --- a/pkg/accessreview/drivers/testdata/google_analytics.yaml +++ b/pkg/accessreview/drivers/testdata/google_analytics.yaml @@ -3,9 +3,12 @@ # the recorder). The driver lists account-level accessBindings, then the # properties beneath the account, then each property's accessBindings, merging a # user's roles across levels by email. alice appears at both levels (admin + -# viewer) to exercise the merge; carol appears only at the property level. The -# AccessBinding shape (name, user, roles[]) and property list shape mirror the -# live API. +# viewer) to exercise the merge; carol appears only at the property level; dave +# appears only on properties/99999, a subproperty parented to another property +# rather than to the account, so the ancestor filter's hierarchy walk is +# covered. properties/55555 returns 403 to cover a property the token cannot +# read being skipped instead of failing the whole account. The AccessBinding +# shape (name, user, roles[]) and property list shape mirror the live API. version: 2 interactions: - id: 0 @@ -59,7 +62,7 @@ interactions: proto_minor: 0 content_length: -1 uncompressed: true - body: '{"properties":[{"name":"properties/67890","displayName":"Acme Website","parent":"accounts/123456"}]}' + body: '{"properties":[{"name":"properties/67890","displayName":"Acme Website","parent":"accounts/123456"},{"name":"properties/99999","displayName":"Acme Rollup","parent":"properties/67890"},{"name":"properties/55555","displayName":"Acme Restricted","parent":"accounts/123456"}]}' headers: Content-Type: - application/json @@ -94,3 +97,59 @@ interactions: status: 200 OK code: 200 duration: 100ms + - id: 3 + request: + proto: HTTP/1.1 + proto_major: 1 + proto_minor: 1 + content_length: 0 + host: analyticsadmin.googleapis.com + form: + pageSize: + - "200" + headers: + Accept: + - application/json + url: https://analyticsadmin.googleapis.com/v1alpha/properties/99999/accessBindings?pageSize=200 + method: GET + response: + proto: HTTP/2.0 + proto_major: 2 + proto_minor: 0 + content_length: -1 + uncompressed: true + body: '{"accessBindings":[{"name":"properties/99999/accessBindings/mno345","user":"dave@example.com","roles":["predefinedRoles/analyst"]}]}' + headers: + Content-Type: + - application/json + status: 200 OK + code: 200 + duration: 100ms + - id: 4 + request: + proto: HTTP/1.1 + proto_major: 1 + proto_minor: 1 + content_length: 0 + host: analyticsadmin.googleapis.com + form: + pageSize: + - "200" + headers: + Accept: + - application/json + url: https://analyticsadmin.googleapis.com/v1alpha/properties/55555/accessBindings?pageSize=200 + method: GET + response: + proto: HTTP/2.0 + proto_major: 2 + proto_minor: 0 + content_length: -1 + uncompressed: true + body: '{"error":{"code":403,"message":"User does not have sufficient permissions for this property.","status":"PERMISSION_DENIED"}}' + headers: + Content-Type: + - application/json + status: 403 Forbidden + code: 403 + duration: 100ms diff --git a/pkg/connector/provider/google_analytics.go b/pkg/connector/provider/google_analytics.go index 18a03eb9b..db04d9085 100644 --- a/pkg/connector/provider/google_analytics.go +++ b/pkg/connector/provider/google_analytics.go @@ -50,7 +50,12 @@ func googleAnalyticsRegistration() *Registration { "https://www.googleapis.com/auth/analytics.readonly", "https://www.googleapis.com/auth/analytics.manage.users.readonly", }, - ProbeURL: "https://analyticsadmin.googleapis.com/v1alpha/accounts?pageSize=1", + // BuildProbeURL targets the selected account's accessBindings rather + // than the accounts list: listing accounts only needs + // analytics.readonly, so a non-Administrator connection (or one where + // the user declined manage.users.readonly on Google's granular consent + // screen) would probe green and then 403 on every fetch. + BuildProbeURL: buildGoogleAnalyticsProbeURL, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { s, err := coredata.ConnectorSettings[coredata.GoogleAnalyticsConnectorSettings](conn) if err != nil { diff --git a/pkg/connector/provider/probe.go b/pkg/connector/provider/probe.go index 3863409e8..28783362b 100644 --- a/pkg/connector/provider/probe.go +++ b/pkg/connector/provider/probe.go @@ -642,6 +642,22 @@ func buildSegmentProbeURL(conn *coredata.Connector) (string, error) { return u.String(), nil } +// buildGoogleAnalyticsProbeURL targets the selected account's accessBindings, +// the driver's first call, so the probe fails for a connection that can list +// accounts but cannot read access bindings. +func buildGoogleAnalyticsProbeURL(conn *coredata.Connector) (string, error) { + s, err := coredata.ConnectorSettings[coredata.GoogleAnalyticsConnectorSettings](conn) + if err != nil { + return "", fmt.Errorf("cannot read google analytics connector settings: %w", err) + } + + if s.AccountID == "" { + return "", fmt.Errorf("missing google analytics account ID") + } + + return drivers.GoogleAnalyticsAccountBindingsProbeURL(s.AccountID) +} + // probeSquare checks a Square credential (OAuth Bearer token or Personal Access // Token) with a GET /v2/merchants/me, sending the required Square-Version // header. The endpoint returns 401 on a dead token and works for both OAuth and