From 8d8f38fb4ca6e767f266ceffa07244c9c55448c2 Mon Sep 17 00:00:00 2001 From: Stefan Benz <46600784+stebenz@users.noreply.github.com> Date: Tue, 7 Jan 2025 17:34:59 +0100 Subject: [PATCH] fix: only allowed idps in login step (#9136) # Which Problems Are Solved If a not allowed IDP is selected or now not allowed IDP was selected before at login, the login will still try to use it as fallback. The same goes for the linked IDPs which are not necessarily active anymore, or disallowed through policies. # How the Problems Are Solved Check all possible or configured IDPs if they can be used. # Additional Changes None # Additional Context Addition to #6466 --------- Co-authored-by: Livio Spring --- .../eventsourcing/eventstore/auth_request.go | 41 +++- .../eventstore/auth_request_test.go | 226 +++++++++++++++++- 2 files changed, 259 insertions(+), 8 deletions(-) diff --git a/internal/auth/repository/eventsourcing/eventstore/auth_request.go b/internal/auth/repository/eventsourcing/eventstore/auth_request.go index e35e7b5143..813c5668f4 100644 --- a/internal/auth/repository/eventsourcing/eventstore/auth_request.go +++ b/internal/auth/repository/eventsourcing/eventstore/auth_request.go @@ -1065,8 +1065,10 @@ func (repo *AuthRequestRepo) nextSteps(ctx context.Context, request *domain.Auth return nil, err } noLocalAuth := request.LoginPolicy != nil && !request.LoginPolicy.AllowUsernamePassword - if (!isInternalLogin || len(idps.Links) > 0 || noLocalAuth) && len(request.LinkingUsers) == 0 { - step, err := repo.idpChecked(request, idps.Links, userSession) + + allowedLinkedIDPs := checkForAllowedIDPs(request.AllowedExternalIDPs, idps.Links) + if (!isInternalLogin || len(allowedLinkedIDPs) > 0 || noLocalAuth) && len(request.LinkingUsers) == 0 { + step, err := repo.idpChecked(request, allowedLinkedIDPs, userSession) if err != nil { return nil, err } @@ -1146,6 +1148,19 @@ func (repo *AuthRequestRepo) nextSteps(ctx context.Context, request *domain.Auth return append(steps, &domain.RedirectToCallbackStep{}), nil } +func checkForAllowedIDPs(allowedIDPs []*domain.IDPProvider, idps []*query.IDPUserLink) (_ []string) { + allowedLinkedIDPs := make([]string, 0, len(idps)) + // only use allowed linked idps + for _, idp := range idps { + for _, allowedIdP := range allowedIDPs { + if idp.IDPID == allowedIdP.IDPConfigID { + allowedLinkedIDPs = append(allowedLinkedIDPs, allowedIdP.IDPConfigID) + } + } + } + return allowedLinkedIDPs +} + func passwordAgeChangeRequired(policy *domain.PasswordAgePolicy, changed time.Time) bool { if policy == nil || policy.MaxAgeDays == 0 { return false @@ -1299,7 +1314,7 @@ func (repo *AuthRequestRepo) firstFactorChecked(ctx context.Context, request *do return &domain.PasswordStep{} } -func (repo *AuthRequestRepo) idpChecked(request *domain.AuthRequest, idps []*query.IDPUserLink, userSession *user_model.UserSessionView) (domain.NextStep, error) { +func (repo *AuthRequestRepo) idpChecked(request *domain.AuthRequest, idps []string, userSession *user_model.UserSessionView) (domain.NextStep, error) { if checkVerificationTimeMaxAge(userSession.ExternalLoginVerification, request.LoginPolicy.ExternalLoginCheckLifetime, request) { request.IDPLoginChecked = true request.AuthTime = userSession.ExternalLoginVerification @@ -1307,15 +1322,27 @@ func (repo *AuthRequestRepo) idpChecked(request *domain.AuthRequest, idps []*que } // use the explicitly set IdP first if request.SelectedIDPConfigID != "" { - return &domain.ExternalLoginStep{SelectedIDPConfigID: request.SelectedIDPConfigID}, nil + // only use the explicitly set IdP if allowed + for _, allowedIdP := range request.AllowedExternalIDPs { + if request.SelectedIDPConfigID == allowedIdP.IDPConfigID { + return &domain.ExternalLoginStep{SelectedIDPConfigID: request.SelectedIDPConfigID}, nil + } + } + // error if the explicitly set IdP is not allowed, to avoid misinterpretation with usage of another IdP + return nil, zerrors.ThrowPreconditionFailed(nil, "LOGIN-LWif2", "Errors.Org.IdpNotExisting") } // reuse the previously used IdP from the session if userSession.SelectedIDPConfigID != "" { - return &domain.ExternalLoginStep{SelectedIDPConfigID: userSession.SelectedIDPConfigID}, nil + // only use the previously used IdP if allowed + for _, allowedIdP := range request.AllowedExternalIDPs { + if userSession.SelectedIDPConfigID == allowedIdP.IDPConfigID { + return &domain.ExternalLoginStep{SelectedIDPConfigID: userSession.SelectedIDPConfigID}, nil + } + } } - // then use an existing linked IdP of the user + // then use an existing linked and allowed IdP of the user if len(idps) > 0 { - return &domain.ExternalLoginStep{SelectedIDPConfigID: idps[0].IDPID}, nil + return &domain.ExternalLoginStep{SelectedIDPConfigID: idps[0]}, nil } // if the user did not link one, then just use one of the configured IdPs of the org if len(request.AllowedExternalIDPs) > 0 { diff --git a/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go b/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go index dda8c54872..976ae8d8a9 100644 --- a/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go +++ b/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go @@ -1247,6 +1247,7 @@ func TestAuthRequestRepo_nextSteps(t *testing.T) { args{&domain.AuthRequest{ UserID: "UserID", SelectedIDPConfigID: "IDPConfigID", + AllowedExternalIDPs: []*domain.IDPProvider{{IDPConfigID: "IDPConfigID"}}, LoginPolicy: &domain.LoginPolicy{ AllowUsernamePassword: false, SecondFactorCheckLifetime: 18 * time.Hour, @@ -1254,6 +1255,193 @@ func TestAuthRequestRepo_nextSteps(t *testing.T) { []domain.NextStep{&domain.ExternalLoginStep{SelectedIDPConfigID: "IDPConfigID"}}, nil, }, + { + "external user (idp selected, not allowed, no external verification), error", + fields{ + userSessionViewProvider: &mockViewUserSession{ + SecondFactorVerification: testNow.Add(-5 * time.Minute), + }, + userViewProvider: &mockViewUser{ + IsEmailVerified: true, + MFAMaxSetUp: int32(domain.MFALevelSecondFactor), + }, + userEventProvider: &mockEventUser{}, + lockoutPolicyProvider: &mockLockoutPolicy{ + policy: &query.LockoutPolicy{ + ShowFailures: true, + }, + }, + orgViewProvider: &mockViewOrg{State: domain.OrgStateActive}, + loginPolicyProvider: &mockLoginPolicy{ + policy: &query.LoginPolicy{ + SecondFactorCheckLifetime: database.Duration(18 * time.Hour), + }, + }, + idpUserLinksProvider: &mockIDPUserLinks{}, + }, + args{&domain.AuthRequest{ + UserID: "UserID", + SelectedIDPConfigID: "IDPConfigID", + AllowedExternalIDPs: []*domain.IDPProvider{}, + LoginPolicy: &domain.LoginPolicy{ + AllowUsernamePassword: false, + SecondFactorCheckLifetime: 18 * time.Hour, + }}, false}, + nil, + zerrors.IsPreconditionFailed, + }, + { + "external user (idp link, no external verification), external login step", + fields{ + userSessionViewProvider: &mockViewUserSession{ + SecondFactorVerification: testNow.Add(-5 * time.Minute), + }, + userViewProvider: &mockViewUser{ + IsEmailVerified: true, + MFAMaxSetUp: int32(domain.MFALevelSecondFactor), + }, + userEventProvider: &mockEventUser{}, + lockoutPolicyProvider: &mockLockoutPolicy{ + policy: &query.LockoutPolicy{ + ShowFailures: true, + }, + }, + orgViewProvider: &mockViewOrg{State: domain.OrgStateActive}, + loginPolicyProvider: &mockLoginPolicy{ + policy: &query.LoginPolicy{ + SecondFactorCheckLifetime: database.Duration(18 * time.Hour), + }, + }, + idpUserLinksProvider: &mockIDPUserLinks{ + []*query.IDPUserLink{ + {IDPID: "IDPConfigID"}, + }, + }, + }, + args{&domain.AuthRequest{ + UserID: "UserID", + SelectedIDPConfigID: "", + AllowedExternalIDPs: []*domain.IDPProvider{{IDPConfigID: "IDPConfigID"}}, + LoginPolicy: &domain.LoginPolicy{ + AllowUsernamePassword: false, + SecondFactorCheckLifetime: 18 * time.Hour, + }}, false}, + []domain.NextStep{&domain.ExternalLoginStep{SelectedIDPConfigID: "IDPConfigID"}}, + nil, + }, + { + "external user (idp link not allowed, no external verification), external login step", + fields{ + userSessionViewProvider: &mockViewUserSession{ + SecondFactorVerification: testNow.Add(-5 * time.Minute), + }, + userViewProvider: &mockViewUser{ + IsEmailVerified: true, + MFAMaxSetUp: int32(domain.MFALevelSecondFactor), + }, + userEventProvider: &mockEventUser{}, + lockoutPolicyProvider: &mockLockoutPolicy{ + policy: &query.LockoutPolicy{ + ShowFailures: true, + }, + }, + orgViewProvider: &mockViewOrg{State: domain.OrgStateActive}, + loginPolicyProvider: &mockLoginPolicy{ + policy: &query.LoginPolicy{ + SecondFactorCheckLifetime: database.Duration(18 * time.Hour), + }, + }, + idpUserLinksProvider: &mockIDPUserLinks{ + []*query.IDPUserLink{ + {IDPID: "IDPConfigID1"}, + }, + }, + }, + args{&domain.AuthRequest{ + UserID: "UserID", + SelectedIDPConfigID: "", + AllowedExternalIDPs: []*domain.IDPProvider{{IDPConfigID: "IDPConfigID2"}}, + LoginPolicy: &domain.LoginPolicy{ + AllowUsernamePassword: false, + SecondFactorCheckLifetime: 18 * time.Hour, + }}, false}, + []domain.NextStep{&domain.ExternalLoginStep{SelectedIDPConfigID: "IDPConfigID2"}}, + nil, + }, + { + "external user (idp link not allowed, none allowed, no external verification), external login step", + fields{ + userSessionViewProvider: &mockViewUserSession{ + SecondFactorVerification: testNow.Add(-5 * time.Minute), + }, + userViewProvider: &mockViewUser{ + IsEmailVerified: true, + MFAMaxSetUp: int32(domain.MFALevelSecondFactor), + }, + userEventProvider: &mockEventUser{}, + lockoutPolicyProvider: &mockLockoutPolicy{ + policy: &query.LockoutPolicy{ + ShowFailures: true, + }, + }, + orgViewProvider: &mockViewOrg{State: domain.OrgStateActive}, + loginPolicyProvider: &mockLoginPolicy{ + policy: &query.LoginPolicy{ + SecondFactorCheckLifetime: database.Duration(18 * time.Hour), + }, + }, + idpUserLinksProvider: &mockIDPUserLinks{ + []*query.IDPUserLink{ + {IDPID: "IDPConfigID1"}, + }, + }, + }, + args{&domain.AuthRequest{ + UserID: "UserID", + SelectedIDPConfigID: "", + AllowedExternalIDPs: []*domain.IDPProvider{}, + LoginPolicy: &domain.LoginPolicy{ + AllowUsernamePassword: false, + SecondFactorCheckLifetime: 18 * time.Hour, + }}, false}, + nil, + zerrors.IsPreconditionFailed, + }, + { + "external user (no idp allowed, no external verification), error", + fields{ + userSessionViewProvider: &mockViewUserSession{ + SecondFactorVerification: testNow.Add(-5 * time.Minute), + }, + userViewProvider: &mockViewUser{ + IsEmailVerified: true, + MFAMaxSetUp: int32(domain.MFALevelSecondFactor), + }, + userEventProvider: &mockEventUser{}, + lockoutPolicyProvider: &mockLockoutPolicy{ + policy: &query.LockoutPolicy{ + ShowFailures: true, + }, + }, + orgViewProvider: &mockViewOrg{State: domain.OrgStateActive}, + loginPolicyProvider: &mockLoginPolicy{ + policy: &query.LoginPolicy{ + SecondFactorCheckLifetime: database.Duration(18 * time.Hour), + }, + }, + idpUserLinksProvider: &mockIDPUserLinks{}, + }, + args{&domain.AuthRequest{ + UserID: "UserID", + SelectedIDPConfigID: "", + AllowedExternalIDPs: []*domain.IDPProvider{}, + LoginPolicy: &domain.LoginPolicy{ + AllowUsernamePassword: false, + SecondFactorCheckLifetime: 18 * time.Hour, + }}, false}, + nil, + zerrors.IsPreconditionFailed, + }, { "external user (only idp available, no external verification), external login step", fields{ @@ -1281,13 +1469,49 @@ func TestAuthRequestRepo_nextSteps(t *testing.T) { }, }, args{&domain.AuthRequest{ - UserID: "UserID", + UserID: "UserID", + AllowedExternalIDPs: []*domain.IDPProvider{{IDPConfigID: "IDPConfigID"}}, LoginPolicy: &domain.LoginPolicy{ AllowUsernamePassword: false, SecondFactorCheckLifetime: 18 * time.Hour, }}, false}, []domain.NextStep{&domain.ExternalLoginStep{SelectedIDPConfigID: "IDPConfigID"}}, nil, + }, { + "external user (only idp available, no allowed, no external verification), external login step", + fields{ + userSessionViewProvider: &mockViewUserSession{ + SecondFactorVerification: testNow.Add(-5 * time.Minute), + }, + userViewProvider: &mockViewUser{ + IsEmailVerified: true, + MFAMaxSetUp: int32(domain.MFALevelSecondFactor), + }, + userEventProvider: &mockEventUser{}, + lockoutPolicyProvider: &mockLockoutPolicy{ + policy: &query.LockoutPolicy{ + ShowFailures: true, + }, + }, + orgViewProvider: &mockViewOrg{State: domain.OrgStateActive}, + loginPolicyProvider: &mockLoginPolicy{ + policy: &query.LoginPolicy{ + SecondFactorCheckLifetime: database.Duration(18 * time.Hour), + }, + }, + idpUserLinksProvider: &mockIDPUserLinks{ + idps: []*query.IDPUserLink{{IDPID: "IDPConfigID"}}, + }, + }, + args{&domain.AuthRequest{ + UserID: "UserID", + AllowedExternalIDPs: []*domain.IDPProvider{}, + LoginPolicy: &domain.LoginPolicy{ + AllowUsernamePassword: false, + SecondFactorCheckLifetime: 18 * time.Hour, + }}, false}, + nil, + zerrors.IsPreconditionFailed, }, { "external user (external verification set), callback",