Skip to content

Commit

Permalink
chore: update audit messages and comments
Browse files Browse the repository at this point in the history
  • Loading branch information
Benehiko committed Oct 12, 2023
1 parent b6873c6 commit ced735e
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 34 deletions.
10 changes: 5 additions & 5 deletions selfservice/flow/registration/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
19 changes: 3 additions & 16 deletions selfservice/strategy/oidc/strategy_login.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
14 changes: 1 addition & 13 deletions selfservice/strategy/oidc/strategy_registration.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down

0 comments on commit ced735e

Please sign in to comment.