diff --git a/selfservice/flow/registration/handler_test.go b/selfservice/flow/registration/handler_test.go index a7c43c31da31..27df3e0868a5 100644 --- a/selfservice/flow/registration/handler_test.go +++ b/selfservice/flow/registration/handler_test.go @@ -382,16 +382,16 @@ func TestGetFlow(t *testing.T) { }) } -// TODO(Benehiko): this test will be updated when the `oidc` strategy is fixed. -// the OIDC strategy incorrectly assumes that is should continue if no -// method is specified but the provider is set. -func TestMultipleStrategies(t *testing.T) { +// This test verifies that the password method is still executed even if the +// oidc strategy is ordered before the password strategy +// when submitting the form with both `method=password` and `provider=google`. +func TestOIDCStrategyOrder(t *testing.T) { t.Logf("This test has been set up to validate the current incorrect `oidc` behaviour. When submitting the form, the `oidc` strategy is executed first, even if the method is set to `password`.") ctx := context.Background() conf, reg := internal.NewFastRegistryWithMocks(t) - // we need to replicate the oidc strategy before the password strategy + // reorder the strategies reg.WithSelfserviceStrategies(t, []any{ oidc.NewStrategy(reg), password.NewStrategy(reg), diff --git a/selfservice/strategy/oidc/strategy_login.go b/selfservice/strategy/oidc/strategy_login.go index 6ac4cc2c7389..c81537015bc9 100644 --- a/selfservice/strategy/oidc/strategy_login.go +++ b/selfservice/strategy/oidc/strategy_login.go @@ -202,29 +202,16 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow, return nil, errors.WithStack(flow.ErrStrategyNotResponsible) } - if p.Method == "" { + if !strings.EqualFold(strings.ToLower(p.Method), s.SettingsStrategyID()) && p.Method != "" { // the user is sending a method that is not oidc, but the payload includes a provider s.d.Audit(). WithRequest(r). WithField("provider", p.Provider). - Warn("The payload includes a `provider` field but does not specify the `method` field. This is incorrect behavior and will be removed in the future.") - } - - // This is a small check to ensure users do not encounter issues with the current "incorrect" oidc behavior. - // this will be removed in the future when the oidc method behavior is fixed. - if !strings.EqualFold(strings.ToLower(p.Method), s.SettingsStrategyID()) && p.Method != "" { - if pid != "" { - s.d.Audit(). - WithRequest(r). - WithField("provider", p.Provider). - WithField("method", p.Method). - Warn("The payload includes a `provider` field but does not specify the `method` field or does not use the `oidc` method. This is incorrect behavior and will be removed in the future.") - } + WithField("method", p.Method). + Warn("The payload includes a `provider` field but is using a method other than `oidc`. Therefore, social sign in will not be executed.") return nil, errors.WithStack(flow.ErrStrategyNotResponsible) } - // TODO(Benehiko): Change the following line to actually match the payload `method` field with the current strategy `oidc`. - // right now it matches itself so it will always be true if err := flow.MethodEnabledAndAllowed(ctx, f.GetFlowName(), s.SettingsStrategyID(), s.SettingsStrategyID(), s.d); err != nil { return nil, s.handleError(w, r, f, pid, nil, err) } diff --git a/selfservice/strategy/oidc/strategy_registration.go b/selfservice/strategy/oidc/strategy_registration.go index 12d95185bc03..b6747ef9ad0a 100644 --- a/selfservice/strategy/oidc/strategy_registration.go +++ b/selfservice/strategy/oidc/strategy_registration.go @@ -169,28 +169,16 @@ func (s *Strategy) Register(w http.ResponseWriter, r *http.Request, f *registrat return errors.WithStack(flow.ErrStrategyNotResponsible) } - if p.Method == "" { - // the user is sending a method that is not oidc, but the payload includes a provider - s.d.Audit(). - WithRequest(r). - WithField("provider", p.Provider). - Warn("The payload includes a `provider` field but does not specify the `method` field. This is incorrect behavior and will be removed in the future.") - } - - // This is a small check to ensure users do not encounter issues with the current "incorrect" oidc behavior. - // this will be removed in the future when the oidc method behavior is fixed. if !strings.EqualFold(strings.ToLower(p.Method), s.SettingsStrategyID()) && p.Method != "" { // the user is sending a method that is not oidc, but the payload includes a provider s.d.Audit(). WithRequest(r). WithField("provider", p.Provider). WithField("method", p.Method). - Warn("The payload includes a `provider` field but specifies a `method` other than `oidc`. This is incorrect behavior and will be removed in the future.") + Warn("The payload includes a `provider` field but is using a method other than `oidc`. Therefore, social sign in will not be executed.") return errors.WithStack(flow.ErrStrategyNotResponsible) } - // TODO(Benehiko): Change the following line to actually match the payload `method` field with the current strategy `oidc`. - // right now it matches itself so it will always be true if err := flow.MethodEnabledAndAllowed(ctx, f.GetFlowName(), s.SettingsStrategyID(), s.SettingsStrategyID(), s.d); err != nil { return s.handleError(w, r, f, pid, nil, err) }