diff --git a/contrib/helm/charts/probo/templates/deployment.yaml b/contrib/helm/charts/probo/templates/deployment.yaml index ba07344f2..e446d3900 100644 --- a/contrib/helm/charts/probo/templates/deployment.yaml +++ b/contrib/helm/charts/probo/templates/deployment.yaml @@ -287,16 +287,20 @@ spec: {{- if has .name $accessReviewProviders }} {{- $envPrefix := printf "CONNECTOR_%s" (.name | upper) }} {{- $secretPrefix := printf "connector-%s" .name }} + {{- if .config.clientId }} - name: {{ $envPrefix }}_CLIENT_ID valueFrom: secretKeyRef: name: {{ include "probo.fullname" $ }} key: {{ $secretPrefix }}-client-id + {{- end }} + {{- if .config.clientSecret }} - name: {{ $envPrefix }}_CLIENT_SECRET valueFrom: secretKeyRef: name: {{ include "probo.fullname" $ }} key: {{ $secretPrefix }}-client-secret + {{- end }} {{- if .config.redirectUri }} - name: {{ $envPrefix }}_REDIRECT_URI value: {{ .config.redirectUri | quote }} diff --git a/pkg/accessreview/drivers/asana.go b/pkg/accessreview/drivers/asana.go index fb5a98d90..428902801 100644 --- a/pkg/accessreview/drivers/asana.go +++ b/pkg/accessreview/drivers/asana.go @@ -79,23 +79,20 @@ func (d *AsanaDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error) } for _, u := range page.Data { - record := AccountRecord{ + // Asana's workspace-users endpoint exposes no active flag, + // and a missing email can mean deactivated, privacy-protected, + // limited-access, or an external collaborator. Inferring + // Active=false from any of those would fabricate state, so + // leave Active nil (unknown) and let downstream review surface + // the gap honestly. + records = append(records, AccountRecord{ Email: u.Email, FullName: u.Name, ExternalID: u.GID, MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessEntryAuthMethodUnknown, AccountType: coredata.AccessEntryAccountTypeUser, - } - - // Asana hides email for deactivated / privacy-protected users. - // We treat missing email as a defensive Active=false signal. - if u.Email == "" { - active := false - record.Active = &active - } - - records = append(records, record) + }) } if page.NextPage == nil || page.NextPage.URI == "" { diff --git a/pkg/accessreview/drivers/clickup.go b/pkg/accessreview/drivers/clickup.go index 9198afb6a..10862ae81 100644 --- a/pkg/accessreview/drivers/clickup.go +++ b/pkg/accessreview/drivers/clickup.go @@ -43,8 +43,13 @@ var _ Driver = (*ClickUpDriver)(nil) func NewClickUpDriver(httpClient *http.Client, teamID string) *ClickUpDriver { return &ClickUpDriver{ - httpClient: httpClient, - teamID: teamID, + httpClient: &http.Client{ + Transport: &retryRoundTripper{ + next: httpClient.Transport, + maxRetries: 3, + }, + }, + teamID: teamID, } } diff --git a/pkg/accessreview/drivers/heroku.go b/pkg/accessreview/drivers/heroku.go index 8c66a92aa..0c81b25ca 100644 --- a/pkg/accessreview/drivers/heroku.go +++ b/pkg/accessreview/drivers/heroku.go @@ -130,8 +130,8 @@ func (d *HerokuDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error return nil, fmt.Errorf("cannot list all heroku accounts: %w", ErrPaginationLimitReached) } -func (d *HerokuDriver) queryMembers(ctx context.Context, url, rangeHeader string) ([]herokuTeamMember, string, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) +func (d *HerokuDriver) queryMembers(ctx context.Context, endpoint, rangeHeader string) ([]herokuTeamMember, string, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { return nil, "", fmt.Errorf("cannot create heroku members request: %w", err) } diff --git a/pkg/accessreview/drivers/monday.go b/pkg/accessreview/drivers/monday.go index 4daeef15a..53ace43ab 100644 --- a/pkg/accessreview/drivers/monday.go +++ b/pkg/accessreview/drivers/monday.go @@ -34,11 +34,9 @@ const mondayGraphQLEndpoint = "https://api.monday.com/v2" const mondayUsersListQuery = `query($p: Int!) { users(limit: 200, page: $p) { id email name enabled is_admin is_guest is_pending last_activity created_at title } }` // MondayDriver fetches users from the Monday.com GraphQL API using a -// pre-authenticated HTTP client. Note: Monday.com's API historically -// accepts a bare token in the Authorization header (no "Bearer " -// prefix), but it also accepts the Bearer-prefixed form produced by -// Probo's RefreshableClient. If a real recording surfaces a 401, swap -// the wire transport for one that strips the "Bearer " prefix. +// pre-authenticated HTTP client. The token flows in the Authorization +// header as a Bearer credential, which Monday.com accepts alongside the +// legacy bare-token form. type MondayDriver struct { httpClient *http.Client } diff --git a/pkg/accessreview/drivers/name_resolver.go b/pkg/accessreview/drivers/name_resolver.go index 0ee051284..6f6ceda20 100644 --- a/pkg/accessreview/drivers/name_resolver.go +++ b/pkg/accessreview/drivers/name_resolver.go @@ -1000,7 +1000,10 @@ func (r *mondayNameResolver) ResolveInstanceName(ctx context.Context) (string, e } if len(resp.Errors) > 0 { - return "", fmt.Errorf("monday graphql error: %s", resp.Errors[0].Message) + // Provider-supplied messages may carry tenant identifiers or + // query fragments — never embed them. Driver scrubs the same + // field; keep both call sites aligned. + return "", fmt.Errorf("cannot fetch monday account: graphql error") } return resp.Data.Account.Name, nil diff --git a/pkg/accessreview/drivers/testdata/vercel.yaml b/pkg/accessreview/drivers/testdata/vercel.yaml index f189894ce..f898ec739 100644 --- a/pkg/accessreview/drivers/testdata/vercel.yaml +++ b/pkg/accessreview/drivers/testdata/vercel.yaml @@ -22,7 +22,7 @@ interactions: proto_minor: 0 content_length: -1 uncompressed: true - body: '{"members":[{"uid":"u_jane","email":"jane@example.com","username":"jane","name":"Jane Doe","role":"OWNER","confirmed":true,"isEnterpriseManaged":false,"joinedFrom":{"origin":"manual"}},{"uid":"u_bob","email":"bob@example.com","username":"bob","name":"Bob Smith","role":"MEMBER","confirmed":false,"isEnterpriseManaged":false,"joinedFrom":{"origin":"invite"}}],"pagination":{"next":""}}' + body: '{"members":[{"uid":"u_jane","email":"jane@example.com","username":"jane","name":"Jane Doe","role":"OWNER","confirmed":true,"isEnterpriseManaged":false,"createdAt":1714564800000,"joinedFrom":{"origin":"manual"}},{"uid":"u_bob","email":"bob@example.com","username":"bob","name":"Bob Smith","role":"MEMBER","confirmed":false,"isEnterpriseManaged":false,"createdAt":1714651200000,"joinedFrom":{"origin":"invite"}}],"pagination":{"count":2,"next":null,"prev":1714564800000}}' headers: Content-Type: - application/json diff --git a/pkg/accessreview/drivers/vercel.go b/pkg/accessreview/drivers/vercel.go index 0b361a1e2..69463c67c 100644 --- a/pkg/accessreview/drivers/vercel.go +++ b/pkg/accessreview/drivers/vercel.go @@ -20,6 +20,7 @@ import ( "fmt" "net/http" "net/url" + "strconv" "go.probo.inc/probo/pkg/coredata" ) @@ -64,10 +65,13 @@ type vercelMember struct { } `json:"joinedFrom"` } +// Vercel's documented pagination shape returns `next` as a Unix-millis +// cursor (number) or null on the last page; modelling it as `*int64` +// matches both. Decoding as a string would fail in production. type vercelMembersPage struct { Members []vercelMember `json:"members"` Pagination struct { - Next string `json:"next"` + Next *int64 `json:"next"` } `json:"pagination"` } @@ -104,10 +108,10 @@ func (d *VercelDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error records = append(records, record) } - if page.Pagination.Next == "" { + if page.Pagination.Next == nil { return records, nil } - cursor = page.Pagination.Next + cursor = strconv.FormatInt(*page.Pagination.Next, 10) } return nil, fmt.Errorf("cannot list all vercel accounts: %w", ErrPaginationLimitReached) diff --git a/pkg/accessreview/review_engine.go b/pkg/accessreview/review_engine.go index 357c77ba0..91f342214 100644 --- a/pkg/accessreview/review_engine.go +++ b/pkg/accessreview/review_engine.go @@ -402,17 +402,11 @@ func (e *ReviewEngine) resolveDriver( } return drivers.NewHerokuDriver(httpClient, herokuSettings.TeamID), nil case coredata.ConnectorProviderPagerDuty: - // Subdomain is required for the name resolver only; the driver - // itself does not need it because PagerDuty's REST API uses the - // regional api.pagerduty.com host. We still surface a clear - // error if the OAuth callback failed to capture the subdomain. - pdSettings, err := coredata.ConnectorSettings[coredata.PagerDutyConnectorSettings](dbConnector) - if err != nil { - return nil, fmt.Errorf("cannot read pagerduty connector settings: %w", err) - } - if pdSettings.Subdomain == "" { - return nil, fmt.Errorf("pagerduty connector requires subdomain in settings") - } + // PagerDuty's REST API uses the regional api.pagerduty.com host; + // the driver does not consume the per-tenant subdomain. Subdomain + // is read only by the name resolver, which returns empty when + // missing — that surfaces as a blank source name but does not + // block access review. return drivers.NewPagerDutyDriver(httpClient), nil case coredata.ConnectorProviderAsana: asanaSettings, err := coredata.ConnectorSettings[coredata.AsanaConnectorSettings](dbConnector) diff --git a/pkg/connector/registry.go b/pkg/connector/registry.go index f3e06bbf9..bb93c4195 100644 --- a/pkg/connector/registry.go +++ b/pkg/connector/registry.go @@ -145,10 +145,10 @@ var ( "NETLIFY": "https://api.netlify.com/api/v1/user", "CLICKUP": "https://api.clickup.com/api/v2/user", "VERCEL": "https://api.vercel.com/v2/user", - // Monday's primary API is GraphQL POST, but the probe handler - // is GET-only. Use the OIDC userinfo endpoint as a GET probe - // that returns 200/401 with the same Bearer token. - "MONDAY": "https://auth.monday.com/oauth2/userinfo", + // Monday's primary API is GraphQL POST, and the auth subdomain + // does not expose a Bearer-protected GET userinfo endpoint, so + // there is no valid probe URL. The probe handler skips empty + // entries; an invalid token surfaces at the next /v2 query. } )