Guard Helm connector env vars on clientId / clientSecret → Address review feedback on access-review drivers

- Guard Helm connector env vars on clientId / clientSecret
- Decode Vercel pagination cursor as *int64
- Drop Monday probe URL — no valid GET endpoint
- Address review feedback on access-review drivers

Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>

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 }}

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 == "" {

pkg/accessreview/drivers/clickup.go

@@ -43,7 +43,12 @@ var _ Driver = (*ClickUpDriver)(nil)
 
 func NewClickUpDriver(httpClient *http.Client, teamID string) *ClickUpDriver {
 	return &ClickUpDriver{
-		httpClient: httpClient,
+		httpClient: &http.Client{
+			Transport: &retryRoundTripper{
+				next:       httpClient.Transport,
+				maxRetries: 3,
+			},
+		},
 		teamID: teamID,
 	}
 }

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) {
+func (d *HerokuDriver) queryMembers(ctx context.Context, endpoint, rangeHeader string) ([]herokuTeamMember, string, error) {
-	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil)
 	if err != nil {
 		return nil, "", fmt.Errorf("cannot create heroku members request: %w", err)
 	}

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
+// pre-authenticated HTTP client. The token flows in the Authorization
-// accepts a bare token in the Authorization header (no "Bearer "
+// header as a Bearer credential, which Monday.com accepts alongside the
-// prefix), but it also accepts the Bearer-prefixed form produced by
+// legacy bare-token form.
-// Probo's RefreshableClient. If a real recording surfaces a 401, swap
-// the wire transport for one that strips the "Bearer " prefix.
 type MondayDriver struct {
 	httpClient *http.Client
 }

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

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

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)

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
+		// PagerDuty's REST API uses the regional api.pagerduty.com host;
-		// itself does not need it because PagerDuty's REST API uses the
+		// the driver does not consume the per-tenant subdomain. Subdomain
-		// regional api.pagerduty.com host. We still surface a clear
+		// is read only by the name resolver, which returns empty when
-		// error if the OAuth callback failed to capture the subdomain.
+		// missing — that surfaces as a blank source name but does not
-		pdSettings, err := coredata.ConnectorSettings[coredata.PagerDutyConnectorSettings](dbConnector)
+		// block access review.
-		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")
-		}
 		return drivers.NewPagerDutyDriver(httpClient), nil
 	case coredata.ConnectorProviderAsana:
 		asanaSettings, err := coredata.ConnectorSettings[coredata.AsanaConnectorSettings](dbConnector)

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
+		// Monday's primary API is GraphQL POST, and the auth subdomain
-		// is GET-only. Use the OIDC userinfo endpoint as a GET probe
+		// does not expose a Bearer-protected GET userinfo endpoint, so
-		// that returns 200/401 with the same Bearer token.
+		// there is no valid probe URL. The probe handler skips empty
-		"MONDAY": "https://auth.monday.com/oauth2/userinfo",
+		// entries; an invalid token surfaces at the next /v2 query.
 	}
 )