SQL-86: OIDC Custom authentication claim#35237
SQL-86: OIDC Custom authentication claim#35237SangJunBak merged 2 commits intoMaterializeInc:mainfrom
Conversation
SangJunBak
requested review from
ohbadiah and
teskje
and removed request for
ohbadiah
|
Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone. PR title guidelines
Pre-merge checklist
|
SangJunBak
force-pushed
the
jun/oidc-auth-claim
branch
7 times, most recently
from
f7bb1c1 to
4312ac7
Compare
- Rather than default to email and sub, we add a system variable to decide which claim in the JWT we treat as the username. This follows the spec in the design doc - Creates a test for requiring the authentication claim to be set - Creates a test for switching the authentication claim and creating a different user based on the claim set
SangJunBak
force-pushed
the
jun/oidc-auth-claim
branch
from
fba07b5 to
b07e033
Compare
teskje
left a comment
teskje
left a comment
There was a problem hiding this comment.
LGTM, just some small comments.
src/authenticator/src/oidc.rs
Outdated
| /// Extract the username from the OIDC claims. | ||
| fn user(&self, authentication_claim: &str) -> Option<&str> { | ||
| match authentication_claim { | ||
| "sub" => Some(&self.sub), |
There was a problem hiding this comment.
Is there a reason for the special treatment of sub? Looks like this field is not used anywhere else, so can't we just handle it like all other authentication_claim values?
src/adapter-types/src/dyncfgs.rs
Outdated
| /// OIDC authentication claim to use as username | ||
| pub const OIDC_AUTHENTICATION_CLAIM: Config<Option<&'static str>> = Config::new( | ||
| "oidc_authentication_claim", | ||
| None, |
There was a problem hiding this comment.
Do you know if there is a reasonable default most users will choose? If so you could put that here to reduce the setup friction a bit.
Might also be that we want the friction to avoid users being surprised by the default, in which case ignore me!
There was a problem hiding this comment.
Do you know if there is a reasonable default most users will choose
I think sub is a reasonable default since the claim will always be there, however most users will want to use their email. Potential issue is we auto-provision the user on login. So in case sub is a random uuid, they'll most likely need to drop that role and configure the claim. But we can always just strongly document this.
- Makes the authentication claim optional but defaults to 'sub' - Adds config variable to test suites
|
TFTR! |
2 participants
Stacked on #35185. The first 2 commits belong to the aforementioned PR.
Motivation
Closes https://linear.app/materializeinc/issue/SQL-86/accept-authentication-claim-config-variablea