CXH-436: skip roles sync on 403

[al-conductorone]

Linear: CXH-436

[linear]

CXH-436 GWS: Skip Roles if permission is denied

[github-actions]

PR Review: CXH-436 - Skip roles sync on 403

• Gather context and read changed files
• Review code changes
• Post review feedback

View job run

[github-actions]

Claude finished @al-conductorone's task in 1m 33s —— View job

---

PR Review: CXH-436 - Skip roles sync on 403

• Gather context and read changed files
• Review code changes
• Post review feedback

---

Summary: This PR gracefully handles 403 (Forbidden) errors during role syncing in three methods — List(), Grants(), and Get() — so the connector can continue syncing other resource types when the service account lacks role management permissions.

Overall: Looks good. The approach is consistent with existing 403 handling in this codebase (e.g., user_actions.go, group.go) and the error messages are clear and actionable. A few observations below:

---

Feedback

1. Inconsistent log levels — intentional but worth a comment (nit)

List() at role.go:57 uses l.Warn, while Grants() at line 125 and Get() at line 253 use l.Info. I can see the rationale (Warn for the primary signal in List, Info for the follow-on messages to reduce noise), and the commit history confirms this was a deliberate choice. This is fine — just noting it's intentional.

2. Get() returning nil resource with nil error — verify SDK tolerance (role.go:256-257)

return nil, nil, nil

List() and Grants() return slices, so nil naturally means "empty set." But Get() returns a single *v2.Resource — a nil resource with no error is semantically ambiguous. If the SDK calls Get() expecting a resource (e.g., during provisioning or resource resolution), a nil resource could lead to a nil pointer dereference downstream. This is probably the most important thing to verify — does the SDK handle a nil resource return from Get() gracefully? If it doesn't, consider returning a wrapped error or a sentinel error the SDK recognizes (like codes.NotFound).

3. Consider logging the 403 response body details (nit)

The googleapi.Error type includes a Message field and sometimes Details. You're already logging zap.Error(err) which should capture the full error string, so this is likely already covered — just flagging in case the string representation truncates details.

4. Grants() restructure is clean (role.go:117-132)

The refactoring from a flat && check to a nested if block to handle both 404 and 403 is clean and readable. No concerns here.

---

Verdict

The only potentially impactful item is #2 — confirming the SDK handles a nil resource from Get() without panicking. Everything else looks solid. Nice clean change.

[btipling]

The request is really to not sync roles when roles are disabled in the c1 UI resource.

The Linear issue asks "Ask: Allow GWS Connector to sync if Roles is denied in api call" We should not do this in my opinion.

I'm not sure this is the requested change since none of the code in this diff would run if roles were disabled. There's a screenshot of the UI in the linear issue https://linear.app/ductone/issue/CXH-436/gws-skip-roles-if-permission-is-denied. There must be some roles API call we are doing even when it is disabled.

We should not skip syncing on 403 but instead return the error

[btipling]

Based on the Linear issue we should only skip "when roles are disabled", which is a C1 UI configuration. It is passed into baton-sdk. We should not use permission errors for this. This will delete data.