[SSO] Add foundation for JWT group-to-role sync#36292
[SSO] Add foundation for JWT group-to-role sync#36292mtabebe wants to merge 1 commit intoMaterializeInc:mainfrom
Conversation
mtabebe
force-pushed
the
ma/sso/scim-foundation
branch
2 times, most recently
from
a51f9fe to
89c42e8
Compare
Add the building blocks for OIDC JWT group-to-role synchronization: mz_jwt_sync builtin sentinel role that marks role memberships managed by JWT sync, distinguishing them from manual grants. Three dyncfg system variables: - oidc_group_role_sync_enabled (bool, default false): feature gate - oidc_group_claim (string, default "groups"): JWT claim name - oidc_group_role_sync_strict (bool, default false): fail-closed mode - OidcClaims::groups() extraction method that normalizes group names (lowercase, deduplicate, sort) from JWT claims. Handles array-of-strings, single-string, and filters non-string values. Returns None for missing claims (skip sync) vs Some(vec![]) for empty claims (revoke all). No behavioral change
mtabebe
force-pushed
the
ma/sso/scim-foundation
branch
from
89c42e8 to
5434073
Compare
SangJunBak
left a comment
SangJunBak
left a comment
There was a problem hiding this comment.
All just nits but otw looks good!
| simple conn=mz_system,user=mz_system | ||
| ALTER SYSTEM SET oidc_group_claim = 'roles' | ||
| ---- | ||
| COMPLETE 0 | ||
|
|
||
| simple conn=mz_system,user=mz_system | ||
| SHOW oidc_group_claim | ||
| ---- | ||
| roles | ||
| COMPLETE 1 | ||
|
|
||
| simple conn=mz_system,user=mz_system | ||
| ALTER SYSTEM SET oidc_group_role_sync_strict = true | ||
| ---- | ||
| COMPLETE 0 | ||
|
|
||
| simple conn=mz_system,user=mz_system | ||
| SHOW oidc_group_role_sync_strict | ||
| ---- | ||
| on | ||
| COMPLETE 1 | ||
|
|
||
| # Reset to defaults | ||
| simple conn=mz_system,user=mz_system | ||
| ALTER SYSTEM RESET oidc_group_role_sync_enabled | ||
| ---- | ||
| COMPLETE 0 | ||
|
|
||
| simple conn=mz_system,user=mz_system | ||
| ALTER SYSTEM RESET oidc_group_claim | ||
| ---- | ||
| COMPLETE 0 | ||
|
|
||
| simple conn=mz_system,user=mz_system | ||
| ALTER SYSTEM RESET oidc_group_role_sync_strict | ||
| ---- | ||
| COMPLETE 0 |
There was a problem hiding this comment.
I wonder how valuable these tests are and whether they belong in this test file?
There was a problem hiding this comment.
I'd imagine us extending these tests in src/environmentd/tests/auth.rs in which we're going to be testing this code path anyways
| pub fn groups(&self, claim_name: &str) -> Option<Vec<String>> { | ||
| let value = self.unknown_claims.get(claim_name)?; | ||
|
|
||
| let raw_groups: Vec<String> = match value { |
There was a problem hiding this comment.
Nit: I wonder if we're being too verbose with the comments? We say the same thing more concisely in the top level comment for each of these comments and the code already reads well
| // Normalize: lowercase for case-insensitive role matching, | ||
| // filter out empty strings (not valid role names), deduplicate | ||
| // via BTreeSet (which also sorts), then collect into a Vec for | ||
| // the caller. Sorted order makes downstream diff computation | ||
| // deterministic. | ||
| let normalized: Vec<String> = raw_groups | ||
| .into_iter() |
There was a problem hiding this comment.
If we're already lowercasing, I wonder if it's worth trimming the whitespace from each end too?
There was a problem hiding this comment.
Nevermind, I just reailzed SQL accepts whitespace in role names
| // Any other JSON type can't represent group names — treat as | ||
| // if the claim were absent so we don't accidentally revoke all | ||
| // roles based on garbage data. | ||
| _ => return None, |
There was a problem hiding this comment.
Wonder if we should log warns each time we expect a group claim to exist but it doesn't?
| // Single-string claim that is whitespace-only: not lowercased into | ||
| // anything meaningful, not filtered (only empty strings are dropped). |
There was a problem hiding this comment.
Re: the trim comment, maybe it makes sense to trim then filter for empty strings?
| } | ||
|
|
||
| #[mz_ore::test] | ||
| fn test_groups_case_insensitive_dedup() { |
There was a problem hiding this comment.
nit: Feels like test_groups_mixed_case already handles this
2 participants
Add the building blocks for OIDC JWT group-to-role synchronization:
mz_jwt_sync builtin sentinel role that marks role memberships managed by JWT sync, distinguishing them from manual grants.
Three dyncfg system variables:
oidc_group_role_sync_enabled (bool, default false): feature gate
oidc_group_claim (string, default "groups"): JWT claim name
oidc_group_role_sync_strict (bool, default false): fail-closed mode
OidcClaims::groups() extraction method that normalizes group names (lowercase, deduplicate, sort) from JWT claims. Handles array-of-strings, single-string, and filters non-string values. Returns None for missing claims (skip sync) vs Some(vec![]) for empty claims (revoke all).
Addresses: SQL-176, SQL-177, SQL-178