fix: handle disabled mfa types correctly during login (#979)

* fix: handle disabled mfa types during login correctly

* fix: add 2fa to default login policy

* fix: setup

* Update internal/setup/step7.go

Co-authored-by: Fabi <38692350+fgerschwiler@users.noreply.github.com>

Co-authored-by: Fabi <38692350+fgerschwiler@users.noreply.github.com>

cmd/zitadel/setup.yaml

@@ -94,4 +94,6 @@ SetUp:
   Step6:
     DefaultLabelPolicy:
       PrimaryColor: '#222324'
-      SecondaryColor: '#ffffff'  
+      SecondaryColor: '#ffffff'
+  Step7:
+    DefaultSecondFactor: 1 #SecondFactorTypeOTP

internal/auth/repository/eventsourcing/eventstore/auth_request.go

@@ -624,12 +624,16 @@ func (repo *AuthRequestRepo) usersForUserSelection(request *model.AuthRequest) (
 
 func (repo *AuthRequestRepo) mfaChecked(userSession *user_model.UserSessionView, request *model.AuthRequest, user *user_model.UserView) (model.NextStep, bool, error) {
 	mfaLevel := request.MfaLevel()
-	promptRequired := (user.MfaMaxSetUp < mfaLevel) || !user.HasRequiredOrgMFALevel(request.LoginPolicy)
+	allowedProviders, required := user.MfaTypesAllowed(mfaLevel, request.LoginPolicy)
-	if promptRequired || !repo.mfaSkippedOrSetUp(user, request.LoginPolicy) {
+	promptRequired := (user.MfaMaxSetUp < mfaLevel) || (len(allowedProviders) == 0 && required)
+	if promptRequired || !repo.mfaSkippedOrSetUp(user) {
 		types := user.MfaTypesSetupPossible(mfaLevel, request.LoginPolicy)
 		if promptRequired && len(types) == 0 {
 			return nil, false, errors.ThrowPreconditionFailed(nil, "LOGIN-5Hm8s", "Errors.Login.LoginPolicy.MFA.ForceAndNotConfigured")
 		}
+		if len(types) == 0 {
+			return nil, true, nil
+		}
 		return &model.MfaPromptStep{
 			Required:     promptRequired,
 			MfaProviders: types,
@@ -639,7 +643,7 @@ func (repo *AuthRequestRepo) mfaChecked(userSession *user_model.UserSessionView,
 	default:
 		fallthrough
 	case model.MFALevelNotSetUp:
-		if user.MfaMaxSetUp == model.MFALevelNotSetUp {
+		if len(allowedProviders) == 0 {
 			return nil, true, nil
 		}
 		fallthrough
@@ -658,11 +662,11 @@ func (repo *AuthRequestRepo) mfaChecked(userSession *user_model.UserSessionView,
 		}
 	}
 	return &model.MfaVerificationStep{
-		MfaProviders: user.MfaTypesAllowed(mfaLevel, request.LoginPolicy),
+		MfaProviders: allowedProviders,
 	}, false, nil
 }
 
-func (repo *AuthRequestRepo) mfaSkippedOrSetUp(user *user_model.UserView, policy *iam_model.LoginPolicyView) bool {
+func (repo *AuthRequestRepo) mfaSkippedOrSetUp(user *user_model.UserView) bool {
 	if user.MfaMaxSetUp > model.MFALevelNotSetUp {
 		return true
 	}

internal/auth/repository/eventsourcing/eventstore/auth_request_test.go

@@ -909,6 +909,25 @@ func TestAuthRequestRepo_mfaChecked(t *testing.T) {
 			false,
 			errors.IsPreconditionFailed,
 		},
+		{
+			"not set up, no mfas configured, no prompt and true",
+			fields{
+				MfaInitSkippedLifeTime: 30 * 24 * time.Hour,
+			},
+			args{
+				request: &model.AuthRequest{
+					LoginPolicy: &iam_model.LoginPolicyView{},
+				},
+				user: &user_model.UserView{
+					HumanView: &user_model.HumanView{
+						MfaMaxSetUp: model.MFALevelNotSetUp,
+					},
+				},
+			},
+			nil,
+			true,
+			nil,
+		},
 		{
 			"not set up, prompt and false",
 			fields{
@@ -988,7 +1007,9 @@ func TestAuthRequestRepo_mfaChecked(t *testing.T) {
 			},
 			args{
 				request: &model.AuthRequest{
-					LoginPolicy: &iam_model.LoginPolicyView{},
+					LoginPolicy: &iam_model.LoginPolicyView{
+						SecondFactors: []iam_model.SecondFactorType{iam_model.SecondFactorTypeOTP},
+					},
 				},
 				user: &user_model.UserView{
 					HumanView: &user_model.HumanView{
@@ -1054,8 +1075,7 @@ func TestAuthRequestRepo_mfaSkippedOrSetUp(t *testing.T) {
 		MfaInitSkippedLifeTime time.Duration
 	}
 	type args struct {
-		user   *user_model.UserView
+		user *user_model.UserView
-		policy *iam_model.LoginPolicyView
 	}
 	tests := []struct {
 		name   string
@@ -1072,9 +1092,6 @@ func TestAuthRequestRepo_mfaSkippedOrSetUp(t *testing.T) {
 						MfaMaxSetUp: model.MFALevelSecondFactor,
 					},
 				},
-				&iam_model.LoginPolicyView{
-					SecondFactors: []iam_model.SecondFactorType{iam_model.SecondFactorTypeOTP},
-				},
 			},
 			true,
 		},
@@ -1090,9 +1107,6 @@ func TestAuthRequestRepo_mfaSkippedOrSetUp(t *testing.T) {
 						MfaInitSkipped: time.Now().UTC().Add(-10 * time.Hour),
 					},
 				},
-				&iam_model.LoginPolicyView{
-					SecondFactors: []iam_model.SecondFactorType{iam_model.SecondFactorTypeOTP},
-				},
 			},
 			true,
 		},
@@ -1108,9 +1122,6 @@ func TestAuthRequestRepo_mfaSkippedOrSetUp(t *testing.T) {
 						MfaInitSkipped: time.Now().UTC().Add(-40 * 24 * time.Hour),
 					},
 				},
-				&iam_model.LoginPolicyView{
-					SecondFactors: []iam_model.SecondFactorType{iam_model.SecondFactorTypeOTP},
-				},
 			},
 			false,
 		},
@@ -1120,7 +1131,7 @@ func TestAuthRequestRepo_mfaSkippedOrSetUp(t *testing.T) {
 			repo := &AuthRequestRepo{
 				MfaInitSkippedLifeTime: tt.fields.MfaInitSkippedLifeTime,
 			}
-			if got := repo.mfaSkippedOrSetUp(tt.args.user, tt.args.policy); got != tt.want {
+			if got := repo.mfaSkippedOrSetUp(tt.args.user); got != tt.want {
 				t.Errorf("mfaSkippedOrSetUp() = %v, want %v", got, tt.want)
 			}
 		})

internal/iam/model/iam.go

@@ -13,6 +13,7 @@ const (
 	Step4
 	Step5
 	Step6
+	Step7
 	//StepCount marks the the length of possible steps (StepCount-1 == last possible step)
 	StepCount
 )

internal/iam/repository/eventsourcing/eventstore.go

@@ -83,10 +83,12 @@ func (es *IAMEventstore) StartSetup(ctx context.Context, iamID string, step iam_
 		return nil, caos_errs.ThrowPreconditionFailed(nil, "EVENT-9so34", "Setup already started")
 	}
 
-	repoIAM := &model.IAM{ObjectRoot: models.ObjectRoot{AggregateID: iamID}, SetUpStarted: model.Step(step)}
+	if iam == nil {
-	if iam != nil {
+		iam = &iam_model.IAM{ObjectRoot: models.ObjectRoot{AggregateID: iamID}}
-		repoIAM.ObjectRoot = iam.ObjectRoot
 	}
+	iam.SetUpStarted = step
+	repoIAM := model.IAMFromModel(iam)
+
 	createAggregate := IAMSetupStartedAggregate(es.AggregateCreator(), repoIAM)
 	err = es_sdk.Push(ctx, es.PushAggregates, repoIAM.AppendEvents, createAggregate)
 	if err != nil {
@@ -603,31 +605,43 @@ func (es *IAMEventstore) RemoveIDPProviderFromLoginPolicy(ctx context.Context, p
 }
 
 func (es *IAMEventstore) AddSecondFactorToLoginPolicy(ctx context.Context, aggregateID string, mfa iam_model.SecondFactorType) (iam_model.SecondFactorType, error) {
-	if mfa == iam_model.SecondFactorTypeUnspecified {
+	repoIAM, addAggregate, err := es.PrepareAddSecondFactorToLoginPolicy(ctx, aggregateID, mfa)
-		return 0, caos_errs.ThrowPreconditionFailed(nil, "EVENT-1M8Js", "Errors.IAM.LoginPolicy.MFA.Unspecified")
-	}
-	iam, err := es.IAMByID(ctx, aggregateID)
 	if err != nil {
 		return 0, err
 	}
-	if _, m := iam.DefaultLoginPolicy.GetSecondFactor(mfa); m != 0 {
+	err = es_sdk.PushAggregates(ctx, es.PushAggregates, repoIAM.AppendEvents, addAggregate)
-		return 0, caos_errs.ThrowAlreadyExists(nil, "EVENT-4Rk09", "Errors.IAM.LoginPolicy.MFA.AlreadyExists")
-	}
-	repoIam := model.IAMFromModel(iam)
-	repoMFA := model.SecondFactorFromModel(mfa)
-
-	addAggregate := LoginPolicySecondFactorAddedAggregate(es.Eventstore.AggregateCreator(), repoIam, repoMFA)
-	err = es_sdk.Push(ctx, es.PushAggregates, repoIam.AppendEvents, addAggregate)
 	if err != nil {
 		return 0, err
 	}
-	es.iamCache.cacheIAM(repoIam)
+	es.iamCache.cacheIAM(repoIAM)
-	if _, m := model.GetMFA(repoIam.DefaultLoginPolicy.SecondFactors, int32(mfa)); m != 0 {
+	if _, m := model.GetMFA(repoIAM.DefaultLoginPolicy.SecondFactors, int32(mfa)); m != 0 {
 		return iam_model.SecondFactorType(m), nil
 	}
 	return 0, caos_errs.ThrowInternal(nil, "EVENT-5N9so", "Errors.Internal")
 }
 
+func (es *IAMEventstore) PrepareAddSecondFactorToLoginPolicy(ctx context.Context, aggregateID string, mfa iam_model.SecondFactorType) (*model.IAM, *models.Aggregate, error) {
+	if mfa == iam_model.SecondFactorTypeUnspecified {
+		return nil, nil, caos_errs.ThrowPreconditionFailed(nil, "EVENT-1M8Js", "Errors.IAM.LoginPolicy.MFA.Unspecified")
+	}
+	iam, err := es.IAMByID(ctx, aggregateID)
+	if err != nil {
+		return nil, nil, err
+	}
+	if _, m := iam.DefaultLoginPolicy.GetSecondFactor(mfa); m != 0 {
+		return nil, nil, caos_errs.ThrowAlreadyExists(nil, "EVENT-4Rk09", "Errors.IAM.LoginPolicy.MFA.AlreadyExists")
+	}
+	repoIAM := model.IAMFromModel(iam)
+	repoMFA := model.SecondFactorFromModel(mfa)
+
+	addAggregate := LoginPolicySecondFactorAddedAggregate(es.Eventstore.AggregateCreator(), repoIAM, repoMFA)
+	aggregate, err := addAggregate(ctx)
+	if err != nil {
+		return nil, nil, err
+	}
+	return repoIAM, aggregate, nil
+}
+
 func (es *IAMEventstore) RemoveSecondFactorFromLoginPolicy(ctx context.Context, aggregateID string, mfa iam_model.SecondFactorType) error {
 	if mfa == iam_model.SecondFactorTypeUnspecified {
 		return caos_errs.ThrowPreconditionFailed(nil, "EVENT-4gJ9s", "Errors.IAM.LoginPolicy.MFA.Unspecified")

internal/setup/config.go

@@ -12,6 +12,7 @@ type IAMSetUp struct {
 	Step4 *Step4
 	Step5 *Step5
 	Step6 *Step6
+	Step7 *Step7
 }
 
 func (setup *IAMSetUp) steps(currentDone iam_model.Step) ([]step, error) {
@@ -25,6 +26,7 @@ func (setup *IAMSetUp) steps(currentDone iam_model.Step) ([]step, error) {
 		setup.Step4,
 		setup.Step5,
 		setup.Step6,
+		setup.Step7,
 	} {
 		if step.step() <= currentDone {
 			continue

internal/setup/step7.go

@@ -0,0 +1,54 @@
+package setup
+
+import (
+	"context"
+
+	"github.com/caos/logging"
+
+	"github.com/caos/zitadel/internal/eventstore/models"
+	es_sdk "github.com/caos/zitadel/internal/eventstore/sdk"
+	iam_model "github.com/caos/zitadel/internal/iam/model"
+	iam_es_model "github.com/caos/zitadel/internal/iam/repository/eventsourcing/model"
+)
+
+type Step7 struct {
+	DefaultSecondFactor iam_model.SecondFactorType
+
+	setup *Setup
+}
+
+func (step *Step7) isNil() bool {
+	return step == nil
+}
+
+func (step *Step7) step() iam_model.Step {
+	return iam_model.Step7
+}
+
+func (step *Step7) init(setup *Setup) {
+	step.setup = setup
+}
+
+func (step *Step7) execute(ctx context.Context) (*iam_model.IAM, error) {
+	iam, agg, err := step.add2FAToPolicy(ctx, step.DefaultSecondFactor)
+	if err != nil {
+		logging.Log("SETUP-ZTuS1").WithField("step", step.step()).WithError(err).Error("unable to finish setup (add default mfa to login policy)")
+		return nil, err
+	}
+	iam, agg, push, err := step.setup.IamEvents.PrepareSetupDone(ctx, iam, agg, step.step())
+	if err != nil {
+		logging.Log("SETUP-OkF8o").WithField("step", step.step()).WithError(err).Error("unable to finish setup (prepare setup done)")
+		return nil, err
+	}
+	err = es_sdk.PushAggregates(ctx, push, iam.AppendEvents, agg)
+	if err != nil {
+		logging.Log("SETUP-YbQ6T").WithField("step", step.step()).WithError(err).Error("unable to finish setup")
+		return nil, err
+	}
+	return iam_es_model.IAMToModel(iam), nil
+}
+
+func (step *Step7) add2FAToPolicy(ctx context.Context, secondFactor iam_model.SecondFactorType) (*iam_es_model.IAM, *models.Aggregate, error) {
+	logging.Log("SETUP-geMGDuZ").Info("adding 2FA to loginPolicy")
+	return step.setup.IamEvents.PrepareAddSecondFactorToLoginPolicy(ctx, step.setup.iamID, secondFactor)
+}

internal/user/model/user_view.go

@@ -142,10 +142,12 @@ func (u *UserView) MfaTypesSetupPossible(level req_model.MFALevel, policy *iam_m
 	return types
 }
 
-func (u *UserView) MfaTypesAllowed(level req_model.MFALevel, policy *iam_model.LoginPolicyView) []req_model.MFAType {
+func (u *UserView) MfaTypesAllowed(level req_model.MFALevel, policy *iam_model.LoginPolicyView) ([]req_model.MFAType, bool) {
 	types := make([]req_model.MFAType, 0)
+	required := true
 	switch level {
 	default:
+		required = policy.ForceMFA
 		fallthrough
 	case req_model.MFALevelSecondFactor:
 		if policy.HasSecondFactors() {
@@ -172,7 +174,7 @@ func (u *UserView) MfaTypesAllowed(level req_model.MFALevel, policy *iam_model.L
 		}
 		//PLANNED: add token
 	}
-	return types
+	return types, required
 }
 
 func (u *UserView) HasRequiredOrgMFALevel(policy *iam_model.LoginPolicyView) bool {