feat: support enterprise SAML email sync via consumed licenses API

[madison-c-evans]

Summary

Resolves CXH-1180

• Detect enterprise-managed SAML via SSO URL, empty provider ID, and explicit error message fallback
• Cache SAML state per-org to support multi-org setups with mixed SAML configurations
• Batch session store writes (SetManyJSON) and reads (GetManyJSON) — zero session calls inside user loops
• Remove dead code: ensureEnterpriseSAMLLoaded, getEnterpriseSAMLEmail, unreachable fallback paths

Test plan

• Verified with real enterprise SAML environment (mevans-test enterprise, mevans-test-org1 and mevans-test-org2)
• Both orgs correctly detected as enterprise SAML (empty provider ID path)
• Enterprise SAML data fetched once, shared across orgs via session store
• All 3 users synced with correct SAML emails from consumed licenses API
• go build ./... and go test ./... pass

🤖 Generated with Claude Code

[linear]

CXH-1180 GitHub: Support syncing emails when SAML is configured at Enterprise level

[github-actions]

Claude finished @madison-c-evans's task in 1m 49s —— View job

---

PR Review: Enterprise SAML Email Sync

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

---

Overall: This is a well-structured refactor that replaces a fragile, reactive SAML detection path with an upfront, per-org state machine. The batched session store usage is a nice improvement. A few items to consider:

---

Issues

1. isEmail validation dropped for enterprise SAML emails (medium)

The old getEnterpriseSAMLEmail method validated that SAMLNameID was a valid email via isEmail() before returning it. The new code at pkg/connector/user.go:259-261 sets userEmail to the raw SAML NameID without checking:

case samlStateEnterprise:
    key := enterpriseSAMLKeyPrefix + strings.ToLower(ghUser.GetLogin())
    if samlEmail, ok := enterpriseSAMLEmails[key]; ok {
        userEmail = samlEmail  // no isEmail() check
    }

SAML NameIDs aren't guaranteed to be email addresses (they can be opaque identifiers). The old code had isEmail(info.SAMLNameID) as a guard. Consider restoring this check, or at minimum documenting why it was intentionally removed.

Fix this →

2. orgSAMLEnabled field on GitHub struct is now unused (low)

The orgSAMLEnabled *bool field at pkg/connector/connector.go:102 is no longer passed to userBuilder (which now manages its own samlStates map). It's only passed to apiTokenBuilder, which accepts it as a parameter but never uses it (the field isn't stored on apiTokenResourceType). This is dead code that should be cleaned up.

Fix this →

3. enterpriseSAMLFetched shared across orgs with mixed SAML (low, worth noting)

The enterpriseSAMLFetched flag at pkg/connector/user.go:115 is a single bool shared across all orgs. If the connector syncs org1 (which is samlStateDisabled) first, then org2 (which is samlStateEnterprise), the fetch will happen when org2 is first encountered — this works correctly. However, if org1 triggers the enterprise path and the fetch fails, the error propagates and enterpriseSAMLFetched stays false, so it will retry on the next page — also correct.

Just noting: the PR description says "fetched once, shared across orgs via session store" — the fetch is guarded per-struct-instance, not per-session. If the SDK creates multiple userResourceType instances (unlikely but possible), the API would be called once per instance. This seems acceptable.

---

Positive observations

• Clean state machine: The samlState enum with checkOrgSAML is much easier to reason about than the old approach of mutating hasSAMLEnabled mid-loop when an error was encountered.
• Batched session store: SetManyJSON / GetManyJSON is a meaningful improvement over per-user session calls.
• Good pagination fix: Starting the consumed-licenses API at page 1 instead of 0 in enterprise_role.go:58 fixes a real duplicate-data bug.
• Defensive validation in validateAppCredentials: The early check at connector.go:231-236 that enterprise licenses are accessible is a nice fail-fast addition, with a clear error message pointing to the PAT requirement.
• Per-org caching: The samlStates map correctly handles multi-org setups where different orgs may have different SAML configurations.

---

Nits

• pkg/connector/user.go:451-452: The filter user.GitHubComSAMLNameID != nil && *user.GitHubComSAMLNameID != "" is good, but you might also want to lowercase-normalize the SAML email for consistency (or at least document that lookups are case-sensitive on the email side but case-insensitive on the login side).
• pkg/connector/user.go:276: The comparison int64(lastGraphQLRateLimit.Remaining) < restApiRateLimit.Remaining compares an int cast to int64 against an int64 — this is fine but the anonymous struct for lastGraphQLRateLimit could be a named type to reduce visual noise.

---

[github-actions]

Claude finished @madison-c-evans's task in 1m 48s —— View job

---

PR Review: Enterprise SAML Email Sync

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

---

Overall: This is a clean refactor that replaces a fragile, reactive SAML fallback path with a well-structured per-org state machine (checkOrgSAML). The batched session store usage (SetManyJSON/GetManyJSON) is a meaningful performance improvement. The pagination fix (page 0 → 1) and the early validateAppCredentials check are good additions.

---

Issues

1. isEmail validation dropped for enterprise SAML emails (medium)

The old getEnterpriseSAMLEmail method validated SAMLNameID via isEmail() before returning it (user.go, old code). The new code at user.go:258-262 sets userEmail to the raw SAML NameID without checking:

case samlStateEnterprise:
    key := enterpriseSAMLKeyPrefix + strings.ToLower(ghUser.GetLogin())
    if samlEmail, ok := enterpriseSAMLEmails[key]; ok {
        userEmail = samlEmail  // no isEmail() check
    }

SAML NameIDs aren't guaranteed to be email addresses — they can be opaque identifiers. The isEmail function already exists in this file (line 289). Consider restoring this guard.

Fix this →

2. orgSAMLEnabled field on GitHub struct is dead code (low)

The orgSAMLEnabled *bool field at connector.go:103 is never assigned a value (always nil). It's passed to apiTokenBuilder at line 125, but that function discards it — the parameter is accepted but not stored on the returned struct (api_token.go:131-136). This is leftover from the refactor. Clean it up.

Fix this →

3. checkOrgSAML treats empty provider ID + enterprises as enterprise SAML (worth noting)

At user.go:392-397, when the GraphQL query succeeds but returns an empty SamlIdentityProvider.Id, the code assumes enterprise SAML if enterprises are configured:

if q.Organization.SamlIdentityProvider.Id == "" {
    if len(u.enterprises) > 0 {
        // → samlStateEnterprise
    }
}

This means any org without SAML configured will trigger the consumed-licenses API fetch when enterprises are configured, even if SAML isn't set up at the enterprise level either. This is probably acceptable since the API will simply return users without SAML data, but it's a behavioral change from the old code that's worth calling out — the old code would have left hasSAMLEnabled = false and used the REST email fallback path instead.

4. Error handling change in the user loop (low, but important to verify)

The old code caught the enterprise SAML error inside the per-user GraphQL loop and gracefully fell back mid-iteration. The new code at user.go:222-224 returns hard errors from per-user SAML queries:

err = u.graphqlClient.Query(ctx, &q, variables)
if err != nil {
    return nil, nil, err  // hard fail, no fallback
}

This is correct because checkOrgSAML should have already classified the org correctly, so this query should never hit the "enterprise SAML" error. But if it does (e.g., GitHub changes the error for a specific user), it would fail the entire sync instead of falling back. This is arguably better behavior (fail-fast vs. silent degradation), but worth noting.

---

Positive observations

• Clean state machine: The samlState enum with checkOrgSAML is much cleaner than the old approach of mutating hasSAMLEnabled mid-loop.
• Batched session store: SetManyJSON/GetManyJSON eliminates per-user session calls — good performance win.
• Pagination fix: Starting at page 1 in enterprise_role.go:58 fixes a real duplicate-data bug.
• Fail-fast validation: The validateAppCredentials check at connector.go:232-236 gives a clear early error for GitHub App installations that can't access enterprise licenses.
• Per-org caching: The samlStates map correctly handles multi-org setups with mixed SAML configurations.
• SSO URL heuristic: Using the SSO URL to detect enterprise-managed SAML (line 405-411) is a clever addition that catches cases where the GraphQL provider ID is present but the actual SAML is enterprise-managed.

---

Nit

• user.go:451-452: The filter stores SAML values by lowercase login (strings.ToLower(user.GitHubComLogin)), and the lookup also lowercases (line 259). Good. However, the SAML email value itself is stored as-is — if case-insensitive email comparison matters downstream, consider normalizing the email too.

---

[github-actions]

Claude finished @madison-c-evans's task in 1m 19s —— View job

---

PR Review: Enterprise SAML Email Sync

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

---

Overall: This is a well-structured refactor. The samlState enum with checkOrgSAML is a significant improvement over the old approach of mutating a shared *bool mid-loop. Batched session store usage and the pagination fix are solid wins. A few items below.

---

Issues

1. SetManyJSON stores raw strings, but GetManyJSON[string] may double-deserialize (medium — verify)

In fetchAndStoreEnterpriseSAML, the samlByLogin map has type map[string]string and is passed to session.SetManyJSON. This will JSON-serialize each value as a quoted string (e.g., "user@example.com"). Then loadEnterpriseSAMLEmails calls session.GetManyJSON[string] to read them back.

This works correctly only if GetManyJSON[string] properly deserializes the JSON string back to a Go string. If the SDK's SetManyJSON/GetManyJSON are symmetric (which they should be), this is fine — but it's worth verifying since the old code used SetManyJSON with *enterpriseEmailInfo structs, and the new code uses plain strings. A quick sanity test with the actual SDK would confirm.

2. checkOrgSAML treats empty provider ID + enterprises as enterprise SAML (low — behavioral change)

At user.go:392-397, when the GraphQL query succeeds but returns an empty SamlIdentityProvider.Id, the code assumes enterprise SAML if enterprises are configured. This means any org without SAML configured will trigger the consumed-licenses API fetch when enterprises exist, even if SAML isn't set up at the enterprise level either. The API will return users without SAML data (so it's safe), but it's extra API calls. The old code would have left SAML disabled and used the REST email fallback.

This is probably acceptable given the PR's goal, but worth noting in case it causes unexpected consumed-licenses API calls for non-SAML enterprise setups.

3. enterpriseSAMLFetched is per-struct-instance, not per-session (low — worth noting)

The PR description says "fetched once, shared across orgs via session store." The fetch is guarded by u.enterpriseSAMLFetched (a struct field), not a session key. If the SDK creates multiple userResourceType instances (unlikely but possible), the API would be called once per instance. This is fine in practice, but the session store does act as the cross-page sharing mechanism — the struct field just prevents redundant API calls within a single instance.

4. Per-user GraphQL errors in samlStateOrgEnabled are now hard failures (low — intentional change)

At user.go:221-224, the old code caught the enterprise SAML error mid-loop and gracefully fell back. The new code returns a hard error:

err = u.graphqlClient.Query(ctx, &q, variables)
if err != nil {
    return nil, nil, err  // hard fail
}

This is correct because checkOrgSAML should have already classified the org. But if GitHub returns the enterprise SAML error for a specific user query (after checkOrgSAML said org-level SAML was fine), it would fail the entire sync. The old code was more resilient here — it would flip the flag and continue. Given the new SSO URL heuristic, this edge case is unlikely, but worth a defensive comment explaining why it's safe.

---

Positive observations

• Clean state machine: samlState enum with checkOrgSAML is much easier to reason about than the old mutable *bool approach.
• Batched session store: SetManyJSON / GetManyJSON eliminates per-user session calls — meaningful performance improvement.
• Pagination fix: Starting at page 1 in enterprise_role.go:58 fixes a real duplicate-data bug.
• Fail-fast validation: The validateAppCredentials check at connector.go:231-236 gives a clear early error for GitHub App installations.
• SSO URL heuristic: Using the SSO URL to detect enterprise-managed SAML (user.go:405-411) is a good addition that catches cases where the provider ID is present but SAML is actually enterprise-managed.
• Dead code cleanup: Removing orgSAMLEnabled from the GitHub struct, ensureEnterpriseSAMLLoaded, and getEnterpriseSAMLEmail is clean.
• isEmail check preserved: I see the isEmail guard is present at user.go:260 (ok && isEmail(samlEmail)) — the prior automated reviews flagged this as missing, but it's there in the current code. Good.

---

Nits

• user.go:405-420: The two strings.Contains(ssoUrl, "/enterprises/") branches could be combined into a single block with len(u.enterprises) > 0 as the inner condition, reducing duplication:

  if strings.Contains(ssoUrl, "/enterprises/") {
      if len(u.enterprises) > 0 {
          // → samlStateEnterprise
      } else {
          // → samlStateDisabled
      }
      return u.samlStates[orgName], nil
  }

• user.go:194-198: The anonymous struct for lastGraphQLRateLimit could be a named type to reduce visual noise, though this is cosmetic.

---

[Bencheng21]

is hasSamlEnabled useless today?

[madison-c-evans]

removed in 9a926e8. It was actually dead in apiTokenBuilder even before this PR (accepted but never stored or used). On the user side, it's replaced by the per-org samlStates map and checkOrgSAML

[btipling]

Just to be clear have we recently introduced something the customer will have to change in order to continue to have their sync succeed? If so we cannot ship without customer success's help.

[github-actions]

Claude finished @madison-c-evans's task in 1m 37s —— View job

---

PR Review: Enterprise SAML Email Sync

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

---

Overall: This is a well-executed refactor. The samlState enum with checkOrgSAML replaces a fragile mutable *bool with a clean, per-org state machine. Batched session store usage, the pagination 0→1 fix, and dead code removal are all solid improvements. A few items below.

---

Issues

1. checkOrgSAML assumes enterprise SAML for any org without a provider when enterprises are configured (medium — behavioral change)

At pkg/connector/user.go:394-399, when the GraphQL query succeeds but returns an empty SamlIdentityProvider.Id, the code assumes enterprise SAML if enterprises are configured:

if q.Organization.SamlIdentityProvider.Id == "" {
    if len(u.enterprises) > 0 {
        // → samlStateEnterprise
    }
}

This means any org without SAML configured will trigger the consumed-licenses API fetch whenever enterprises exist, even if SAML isn't set up at the enterprise level either. The old code would have set hasSAMLEnabled = false and used the REST email fallback path (or the enterprise cache if separately loaded). The new code skips the REST email entirely for these users, since samlStateEnterprise only checks the SAML map.

In practice, the API returns users without SAML data (so it's safe), but orgs with no SAML at all will now lose the REST API public email they previously got. Consider whether the samlStateEnterprise path should fall through to the REST email when no SAML mapping is found, rather than leaving userEmail as-is (which would be the REST API email from ghUser.GetEmail() — actually, looking again, userEmail is initialized from ghUser.GetEmail() at line 211 and only overwritten if a SAML mapping is found at line 262-263, so the fallback is preserved). On closer inspection this is fine — users without SAML mappings keep their REST email. Disregard this as a blocking issue, but the extra consumed-licenses API calls for non-SAML enterprise setups are worth noting.

2. Per-user GraphQL errors in samlStateOrgEnabled are now hard failures (low — intentional, but worth a defensive comment)

At pkg/connector/user.go:223-226, the old code caught the enterprise SAML error mid-loop and gracefully fell back. The new code returns a hard error:

err = u.graphqlClient.Query(ctx, &q, variables)
if err != nil {
    return nil, nil, err
}

This is correct because checkOrgSAML should have already classified the org. But if GitHub returns the enterprise SAML error for a specific user-scoped query (after checkOrgSAML said org-level SAML was fine), it would fail the entire sync. The old code would flip the flag and continue. Given the new SSO URL heuristic, this edge case is unlikely, but a brief comment explaining why it's safe to hard-fail here would help future readers.

Add comment →

3. checkOrgSAML SSO URL branches can be simplified (low — readability)

At pkg/connector/user.go:407-422, the two strings.Contains(ssoUrl, "/enterprises/") branches differ only on whether len(u.enterprises) > 0. These can be combined:

if strings.Contains(ssoUrl, "/enterprises/") {
    if len(u.enterprises) > 0 {
        u.samlStates[orgName] = samlStateEnterprise
    } else {
        u.samlStates[orgName] = samlStateDisabled
    }
    return u.samlStates[orgName], nil
}

Fix this →

4. validateAppCredentials makes a real API call as a probe (low — worth noting)

At pkg/connector/connector.go:231-241, the new validation fetches page 1 of consumed licenses to verify access. This is a reasonable fail-fast check, but it means every connector startup (even for non-enterprise-SAML setups that happen to have enterprises configured) makes an extra API call. The error message mentioning PAT vs GitHub App is helpful.

---

Addressing the review comment

Re: @Bencheng21's question on api_token.go:131 — "is hasSamlEnabled useless today?"

Yes, the hasSamlEnabled parameter was removed in commit 9a926e8. The apiTokenBuilder function no longer accepts it (api_token.go:131), and the GitHub struct no longer has the orgSAMLEnabled field (connector.go). This is clean — the dead code was properly removed.

---

Positive observations

• Clean state machine: The samlState enum with checkOrgSAML is much easier to reason about than the old mutable *bool approach with mid-loop error-driven state transitions.
• Batched session store: SetManyJSON / GetManyJSON with the key index pattern eliminates per-user session calls — meaningful performance improvement.
• isEmail check preserved: The guard is present at user.go:262 (ok && isEmail(samlEmail)) — prior reviews flagged this as missing, but it's there in the current code. Good.
• Pagination fix: Starting at page 1 in enterprise_role.go:58 fixes a real duplicate-data bug.
• Fail-fast validation: validateAppCredentials at connector.go:231-241 gives a clear early error pointing users to the PAT requirement.
• SSO URL heuristic: Using the SSO URL to detect enterprise-managed SAML is a clever addition that catches the case where the provider ID is present but SAML is actually managed at the enterprise level.
• Dead code removed: orgSAMLEnabled, ensureEnterpriseSAMLLoaded, getEnterpriseSAMLEmail, and the enterpriseEmailInfo struct are all cleanly removed.
• Test simplification: Removing the hasSamlEnabled parameter matrix in user_test.go correctly reflects the new design where SAML state is discovered internally.

---

Nit

• user.go:230: In the samlStateOrgEnabled path, userEmail = samlIdent.NameId is set without an isEmail check (unlike the enterprise path at line 262). The old code also didn't check here, so this isn't a regression — but it's an inconsistency worth noting. The NameId could be a non-email SAML identifier, and it would override the valid REST email from ghUser.GetEmail().

---

[btipling]

Just to be clear have we recently introduced something the customer will have to change in order to continue to have their sync succeed? If so we cannot ship without customer success's help.

[btipling]

This is likely incorrect. We should look at what the error is and only wrap with codes.PermissionDenied when we are sure it is actually a permission denied error and not a context cancelled or anything else. An err could be anything.

[madison-c-evans]

re: Just to be clear have we recently introduced something the customer will have to change in order to continue to have their sync succeed? If so we cannot ship without customer success's help.

Upon further review, the new validateAppCredentials enterprise license probe hard-fails. I think best route is downgrade the validation to a warning. If consumed-licenses fails in validateAppCredentials, log a warning and skip enterprise SAML entirely (set a flag like enterpriseSAMLAvailable = false). Sync continues without SAML emails, matching old behavior. The fetchAndStoreEnterpriseSAML path would check this flag and skip

[github-actions]

Claude finished @madison-c-evans's task in 1m 32s —— View job

---

PR Review: Enterprise SAML Email Sync

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

---

Overall: The latest commit (f83f961) addresses btipling's core concern well — validateAppCredentials no longer hard-fails on consumed-licenses errors. The enterpriseLicensesAvailable flag cleanly gates both enterprise role sync and SAML enrichment. The samlState state machine is a significant improvement over the old mutable *bool approach. One remaining issue with the same error-wrapping pattern in enterprise_role.go.

---

Issues

1. enterprise_role.go:63 — same PermissionDenied wrapping issue btipling flagged (medium)

The fillCache method still wraps all ListEnterpriseConsumedLicenses errors as codes.PermissionDenied:

// enterprise_role.go:63
return uhttp.WrapErrors(codes.PermissionDenied, fmt.Sprintf("baton-github: error listing enterprise consumed licenses for %s", enterprise), err)

This has the same problem btipling identified in connector.go — a context cancellation, network timeout, or rate limit error would be misclassified as a permission denied error. Since enterpriseLicensesAvailable guards access (the API was already probed successfully in Validate), errors here are more likely transient. Consider either removing the PermissionDenied wrapper (just return the error directly) or inspecting the error type to choose the correct code.

Fix this →

2. SSO URL branches can be combined (low — readability)

pkg/connector/user.go:415-429 has two separate strings.Contains(ssoUrl, "/enterprises/") checks that differ only on len(u.enterprises) > 0:

if strings.Contains(ssoUrl, "/enterprises/") && len(u.enterprises) > 0 {
    // → samlStateEnterprise
}
if strings.Contains(ssoUrl, "/enterprises/") && len(u.enterprises) == 0 {
    // → samlStateDisabled
}

These can be combined into a single block:

if strings.Contains(ssoUrl, "/enterprises/") {
    if len(u.enterprises) > 0 {
        u.samlStates[orgName] = samlStateEnterprise
    } else {
        u.samlStates[orgName] = samlStateDisabled
    }
    return u.samlStates[orgName], nil
}

Fix this →

3. samlStateOrgEnabled path doesn't validate isEmail on NameId (low — not a regression, but inconsistent)

At pkg/connector/user.go:237, the org-level SAML path sets userEmail = samlIdent.NameId without an isEmail() check. The enterprise path at line 269 correctly guards with isEmail(samlEmail). A non-email SAML NameID would override the valid REST email from ghUser.GetEmail(). The old code had the same gap, so this isn't a regression — but it would be good to fix while you're in here.

Fix this →

---

Positive observations

• Graceful degradation is well-designed: The enterpriseLicensesAvailable flag set during Validate cleanly gates both enterprise role sync and SAML enrichment. The warning messages clearly explain the limitation and the PAT requirement for GitHub App installations.
• Belt-and-suspenders fallback: fetchAndStoreEnterpriseSAML also gracefully falls back to samlStateDisabled if the fetch fails during the user List loop (lines 155-159), so even if the API becomes unavailable mid-sync, it recovers.
• Clean state machine: The samlState enum with checkOrgSAML and per-org caching is much easier to reason about than the old mutable *bool with mid-loop error-driven transitions.
• Batched session store: SetManyJSON / GetManyJSON with the key index pattern eliminates per-user session calls.
• Pagination fix: Starting at page 1 in enterprise_role.go:58 fixes a real duplicate-data bug.
• isEmail check on enterprise path: Correctly preserved at user.go:269.
• Dead code cleanly removed: orgSAMLEnabled field, hasSamlEnabled parameter, and old fallback methods are all gone.
• Duplicated validation logic is consistent: Both Validate (PAT path, line 220-228) and validateAppCredentials (GitHub App path, line 244-253) use the same probe-and-warn pattern with appropriate context in the warning message.

---

Re: btipling's review comments

api_token.go:131 — "have we recently introduced something the customer will have to change?": No. The hasSamlEnabled parameter was dead code even before this PR (accepted but never stored or used in apiTokenBuilder). Its removal is purely cleanup. The behavioral change is in enterprise SAML detection, which now gracefully degrades rather than hard-failing — existing customer configurations will continue to work without changes.

connector.go — error wrapping with PermissionDenied: Addressed in f83f961. The validation now logs a warning and sets enterpriseLicensesAvailable = false instead of returning an error. However, the same pattern persists in enterprise_role.go:63 (see issue #1 above).

---