Skip to content

oidc: skip email_verified check when claim is absent #388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jflecool2 wants to merge 1 commit into
base: main
Choose a base branch
from
+37 −15
Open

oidc: skip email_verified check when claim is absent #388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions internal/service/oidc_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ func (s *OIDCService) HandleCallback(ctx context.Context, in domain.OIDCCallback
var claims struct {
Sub string `json:"sub"`
Email string `json:"email"`
EmailVerified bool `json:"email_verified"`
EmailVerified *bool `json:"email_verified"` // nil when IdP omits the claim (e.g. Azure via Cloudflare Access)
Name string `json:"name"`
Azp string `json:"azp"`
Aud audience `json:"aud"` // tolerates string OR []string
Expand Down Expand Up @@ -314,7 +314,7 @@ func (s *OIDCService) HandleCallback(ctx context.Context, in domain.OIDCCallback

// resolveOrProvisionUser implements the identity/linking state machine.
func (s *OIDCService) resolveOrProvisionUser(
ctx context.Context, issuer, sub, email string, emailVerified bool, name string,
ctx context.Context, issuer, sub, email string, emailVerified *bool, name string,
) (*domain.User, error) {

// (1) Stable identity lookup by (issuer, sub). Email NOT used for auth here.
Expand All @@ -331,8 +331,10 @@ func (s *OIDCService) resolveOrProvisionUser(
return nil, fmt.Errorf("oidc federated lookup: %w", err)
}

// NOT FOUND -> first login. email_verified gate applies to every link/provision.
if !emailVerified {
// NOT FOUND -> first login.
// If the IdP explicitly asserts email_verified=false, reject.
// If the claim is absent (nil), the IdP doesn't support it — skip the check.
if emailVerified != nil && !*emailVerified {
if s.logger != nil {
s.logger.WithField("oidc_sub", sub).WithField("issuer", issuer).
Warn("OIDC first-login rejected: email_verified=false")
Expand Down
42 changes: 31 additions & 11 deletions internal/service/oidc_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ func TestResolve_FoundByIssuerSub_NoEmailLookup(t *testing.T) {
m.userRepo.EXPECT().GetUserByID(gomock.Any(), "u1").Return(&domain.User{ID: "u1"}, nil)
// No GetUserByEmail* expected — would fail the controller if called.

u, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "u1@corp.com", true, "U1")
u, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "u1@corp.com", boolPtr(true), "U1")
require.NoError(t, err)
assert.Equal(t, "u1", u.ID)
}
Expand All @@ -276,10 +276,30 @@ func TestResolve_FirstLogin_EmailNotVerified(t *testing.T) {

m.fedRepo.EXPECT().GetByIssuerSubject(gomock.Any(), testIssuer, "sub-1").Return(nil, notFoundFI())

_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "u1@corp.com", false, "")
_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "u1@corp.com", boolPtr(false), "")
assert.ErrorIs(t, err, domain.ErrOIDCEmailNotVerified)
}

func TestResolve_FirstLogin_EmailVerifiedAbsent_Allowed(t *testing.T) {
// When the IdP omits email_verified entirely (nil), login should proceed —
// e.g. Azure/Entra ID via Cloudflare Access never emits this claim.
ctrl := gomock.NewController(t)
defer ctrl.Finish()
cfg := enabledCfg()
cfg.AutoCreateUsers = true
cfg.AllowedDomains = []string{"corp.com"}
svc, m := newTestOIDCService(t, ctrl, cfg, nil)

m.fedRepo.EXPECT().GetByIssuerSubject(gomock.Any(), testIssuer, "sub-new").Return(nil, notFoundFI())
m.userRepo.EXPECT().GetUserByEmailInsensitive(gomock.Any(), "u1@corp.com").Return(nil, notFoundUsr())
m.userRepo.EXPECT().CreateUser(gomock.Any(), gomock.Any()).Return(nil)
m.fedRepo.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil)

u, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-new", "u1@corp.com", nil, "U1")
require.NoError(t, err)
assert.NotNil(t, u)
}

func TestResolve_FirstLogin_BridgeInvitedUser_CaseInsensitive(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand All @@ -297,7 +317,7 @@ func TestResolve_FirstLogin_BridgeInvitedUser_CaseInsensitive(t *testing.T) {
return nil
})

u, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "jane@corp.com", true, "")
u, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "jane@corp.com", boolPtr(true), "")
require.NoError(t, err)
assert.Equal(t, "u1", u.ID)
}
Expand All @@ -314,7 +334,7 @@ func TestResolve_FirstLogin_LinkConflict_DifferentSubSameIssuer(t *testing.T) {
m.fedRepo.EXPECT().GetByUserAndIssuer(gomock.Any(), "u1", testIssuer).
Return(&domain.FederatedIdentity{UserID: "u1", IDPIssuer: testIssuer, IDPSub: "sub-OLD"}, nil)

_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-NEW", "jane@corp.com", true, "")
_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-NEW", "jane@corp.com", boolPtr(true), "")
assert.ErrorIs(t, err, domain.ErrOIDCIdentityConflict)
}

Expand All @@ -327,7 +347,7 @@ func TestResolve_InvitedOnly_NoAccount_Rejected(t *testing.T) {
m.fedRepo.EXPECT().GetByIssuerSubject(gomock.Any(), testIssuer, "sub-1").Return(nil, notFoundFI())
m.userRepo.EXPECT().GetUserByEmailInsensitive(gomock.Any(), "ghost@corp.com").Return(nil, notFoundUsr())

_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "ghost@corp.com", true, "")
_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "ghost@corp.com", boolPtr(true), "")
assert.ErrorIs(t, err, domain.ErrOIDCAccountNotProvisioned)
}

Expand All @@ -345,7 +365,7 @@ func TestResolve_Bridge_RootEmailGuard_RefusesFirstTimeLink(t *testing.T) {
m.fedRepo.EXPECT().GetByUserAndIssuer(gomock.Any(), "root-id", testIssuer).Return(nil, notFoundFI())
// MUST NOT link or mint: no fedRepo.Create expected.

_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "attacker-sub", "root@corp.com", true, "")
_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "attacker-sub", "root@corp.com", boolPtr(true), "")
assert.ErrorIs(t, err, domain.ErrOIDCAccountNotProvisioned,
"first-time linking a ROOT_EMAIL account must be refused (privilege escalation)")
}
Expand All @@ -362,7 +382,7 @@ func TestResolve_Bridge_RootEmailGuard_CaseInsensitive(t *testing.T) {
Return(&domain.User{ID: "root-id", Email: "Root@Corp.com"}, nil)
m.fedRepo.EXPECT().GetByUserAndIssuer(gomock.Any(), "root-id", testIssuer).Return(nil, notFoundFI())

_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "s", "root@corp.com", true, "")
_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "s", "root@corp.com", boolPtr(true), "")
assert.ErrorIs(t, err, domain.ErrOIDCAccountNotProvisioned)
}

Expand All @@ -377,7 +397,7 @@ func TestResolve_Bridge_AlreadyLinkedRoot_AllowsReLogin(t *testing.T) {
Return(&domain.FederatedIdentity{UserID: "root-id", IDPIssuer: testIssuer, IDPSub: "root-sub"}, nil)
m.userRepo.EXPECT().GetUserByID(gomock.Any(), "root-id").Return(&domain.User{ID: "root-id", Email: "root@corp.com"}, nil)

u, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "root-sub", "root@corp.com", true, "")
u, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "root-sub", "root@corp.com", boolPtr(true), "")
require.NoError(t, err, "an already-linked root identity (found by issuer,sub) must re-login")
assert.Equal(t, "root-id", u.ID)
}
Expand All @@ -395,7 +415,7 @@ func TestResolve_JIT_RootEmailGuard_Refused(t *testing.T) {
m.userRepo.EXPECT().GetUserByEmailInsensitive(gomock.Any(), "root@corp.com").Return(nil, notFoundUsr())
// No CreateUser expected.

_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "root@corp.com", true, "")
_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "root@corp.com", boolPtr(true), "")
assert.ErrorIs(t, err, domain.ErrOIDCAccountNotProvisioned)
}

Expand All @@ -410,7 +430,7 @@ func TestResolve_JIT_DomainNotAllowed(t *testing.T) {
m.fedRepo.EXPECT().GetByIssuerSubject(gomock.Any(), testIssuer, "sub-1").Return(nil, notFoundFI())
m.userRepo.EXPECT().GetUserByEmailInsensitive(gomock.Any(), "x@evil.com").Return(nil, notFoundUsr())

_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "x@evil.com", true, "")
_, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "x@evil.com", boolPtr(true), "")
assert.ErrorIs(t, err, domain.ErrOIDCDomainNotAllowed)
}

Expand All @@ -433,7 +453,7 @@ func TestResolve_JIT_CreatesUserAndLinks(t *testing.T) {
})
m.fedRepo.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil)

u, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "new@corp.com", true, "New User")
u, err := svc.resolveOrProvisionUser(context.Background(), testIssuer, "sub-1", "new@corp.com", boolPtr(true), "New User")
require.NoError(t, err)
assert.Equal(t, "new@corp.com", u.Email)
}
Expand Down