From 728011c0426597c314a109a174ce8d5e7d2c912c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Sibiril?= <81782+aureliensibiril@users.noreply.github.com> Date: Sat, 25 Jul 2026 09:00:13 +0200 Subject: [PATCH] Harden the Google Analytics account fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects, all found reviewing the rebased branch. The connection probe hit /v1alpha/accounts, which any analytics.readonly grant can call, while the driver's first request is the account's accessBindings — that additionally needs Administrator on the account and the manage.users.readonly scope. An Editor connecting, or a user declining the second scope on Google's granular consent screen, probed green and then 403'd on every campaign fetch, leaving the source permanently "Connected" with no rows. The probe now targets the same accessBindings collection the driver reads. A single unreadable property aborted the whole account. A property the token cannot see, or one deleted between the list and the read, threw away every binding already collected; 49 of 50 readable properties are still worth reviewing, so 403 and 404 now skip that property. Anything else still fails the fetch. Fan-out errors named no resource: the account call, the property list and each per-property call all returned the same "unexpected status" string, so a 403 on one subproperty out of forty was unattributable. Errors now carry the account or property ID. The cassette gains a subproperty parented to another property (only reachable through the ancestor filter, so it pins the hierarchy walk that the filter change claimed) and a property returning 403. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> --- pkg/accessreview/drivers/google_analytics.go | 48 +++++++++++-- .../drivers/google_analytics_test.go | 12 +++- pkg/accessreview/drivers/organizations.go | 25 +++---- .../drivers/testdata/google_analytics.yaml | 67 +++++++++++++++++-- pkg/connector/provider/google_analytics.go | 7 +- pkg/connector/provider/probe.go | 16 +++++ 6 files changed, 150 insertions(+), 25 deletions(-) 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