feat(oidc): Make state timeout duration configurable to support long …

…taking sign ins (#362) * feat(oidc): Make state timeout duration configurable to support long taking sign in * chore: CR feedback
i-love-flamingo · Oct 18, 2023 · f410758 · f410758
1 parent 258bd2d
commit f410758
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 16 deletions.
diff --git a/core/auth/oauth/module.go b/core/auth/oauth/module.go
@@ -46,6 +46,7 @@ core: auth: {
 		}
 		enableEndSessionEndpoint: bool | *true
 		overrideIssuerURL: string | *""
+		stateLifeTime: string | *"30m"
 	}
 }
 `

diff --git a/core/auth/oauth/oidc.go b/core/auth/oauth/oidc.go
@@ -58,6 +58,7 @@ type (
 		oidcConfig               oidcConfig
 		verifierConfigurator     []func(*oidc.Config)
 		callbackErrorHandler     CallbackErrorHandler
+		stateTimeout             *time.Duration
 	}
 
 	sessionData struct {
@@ -85,6 +86,7 @@ type (
 		} `json:"claims"`
 		EnableEndSessionEndpoint bool   `json:"enableEndSessionEndpoint"`
 		OverrideIssuerURL        string `json:"overrideIssuerURL"`
+		StateLifeTime            string `json:"stateLifeTime"`
 	}
 )
 
@@ -140,6 +142,15 @@ func oidcFactory(cfg config.Map) (auth.RequestIdentifier, error) {
 		authCodeOptions = append(authCodeOptions, oauth2AuthCodeOption{authCodeOption: authCodeOption})
 	}
 
+	stateTimeout := defaultStateTimeout
+
+	if oidcConfig.StateLifeTime != "" {
+		stateTimeout, err = time.ParseDuration(oidcConfig.StateLifeTime)
+		if err != nil {
+			panic("invalid value for oidc broker config stateLifeTime")
+		}
+	}
+
 	return &openIDIdentifier{
 		oauth2Config: &oauth2.Config{
 			ClientID:     oidcConfig.ClientID,
@@ -152,6 +163,7 @@ func oidcFactory(cfg config.Map) (auth.RequestIdentifier, error) {
 		provider:        provider,
 		authcodeOptions: authCodeOptions,
 		oidcConfig:      oidcConfig,
+		stateTimeout:    &stateTimeout,
 	}, nil
 }
 
@@ -362,7 +374,7 @@ type StateEntry struct {
 	TS    time.Time
 }
 
-const stateTimeout = time.Minute * 30
+const defaultStateTimeout = time.Minute * 30
 
 func init() {
 	gob.Register([]StateEntry(nil))
@@ -374,6 +386,12 @@ const sessionStatesKey = "states"
 var now = time.Now
 
 func (i *openIDIdentifier) validateSessionCode(request *web.Request, code string) bool {
+	stateTimeout := defaultStateTimeout
+
+	if i.stateTimeout != nil {
+		stateTimeout = *i.stateTimeout
+	}
+
 	sessionStates, ok := request.Session().Load(i.sessionCode(sessionStatesKey))
 	if !ok {
 		return false

diff --git a/core/auth/oauth/oidc_test.go b/core/auth/oauth/oidc_test.go
@@ -54,25 +54,27 @@ func (m *mockCallbackErrorHandler) Handle(_ context.Context, _ string, _ *web.Re
 
 var _ CallbackErrorHandler = &mockCallbackErrorHandler{}
 
+//nolint:paralleltest // time.Now lives in global var `now` running this parallel will cause race cond
 func TestParallelStateRaceConditions(t *testing.T) {
-	identifier := &openIDIdentifier{
-		authCodeOptionerProvider: func() []AuthCodeOptioner { return nil },
-		oauth2Config:             &oauth2.Config{},
-		reverseRouter:            new(mockRouter),
-		responder:                &web.Responder{},
-	}
+	//nolint:paralleltest // time.Now lives in global var `now` running this parallel will cause race cond
+	t.Run("test states", func(t *testing.T) {
+		identifier := &openIDIdentifier{
+			authCodeOptionerProvider: func() []AuthCodeOptioner { return nil },
+			oauth2Config:             &oauth2.Config{},
+			reverseRouter:            new(mockRouter),
+			responder:                &web.Responder{},
+		}
 
-	session := web.EmptySession()
+		session := web.EmptySession()
 
-	resp := identifier.Authenticate(context.Background(), web.CreateRequest(nil, session))
-	state1 := resp.(*web.URLRedirectResponse).URL.Query().Get("state")
-	resp = identifier.Authenticate(context.Background(), web.CreateRequest(nil, session))
-	state2 := resp.(*web.URLRedirectResponse).URL.Query().Get("state")
+		resp := identifier.Authenticate(context.Background(), web.CreateRequest(nil, session))
+		state1 := resp.(*web.URLRedirectResponse).URL.Query().Get("state")
+		resp = identifier.Authenticate(context.Background(), web.CreateRequest(nil, session))
+		state2 := resp.(*web.URLRedirectResponse).URL.Query().Get("state")
 
-	request, err := http.NewRequest(http.MethodGet, "http://example.com/callback", nil)
-	assert.NoError(t, err)
+		request, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://example.com/callback", nil)
+		assert.NoError(t, err)
 
-	t.Run("test states", func(t *testing.T) {
 		request.URL.RawQuery = url.Values{"state": []string{"invalid-state"}}.Encode()
 		resp = identifier.Callback(context.Background(), web.CreateRequest(request, session), nil)
 		errResp := resp.(*web.ServerErrorResponse)
@@ -94,7 +96,20 @@ func TestParallelStateRaceConditions(t *testing.T) {
 		assert.EqualError(t, errResp.Error, "state mismatch")
 	})
 
-	t.Run("test timeshift", func(t *testing.T) {
+	//nolint:paralleltest // time.Now lives in global var `now` running this parallel will cause race cond
+	t.Run("test default time shift", func(t *testing.T) {
+		identifier := &openIDIdentifier{
+			authCodeOptionerProvider: func() []AuthCodeOptioner { return nil },
+			oauth2Config:             &oauth2.Config{},
+			reverseRouter:            new(mockRouter),
+			responder:                &web.Responder{},
+		}
+
+		request, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://example.com/callback", nil)
+		assert.NoError(t, err)
+
+		session := web.EmptySession()
+
 		resp := identifier.Authenticate(context.Background(), web.CreateRequest(nil, session))
 		state1 := resp.(*web.URLRedirectResponse).URL.Query().Get("state")
 
@@ -109,6 +124,37 @@ func TestParallelStateRaceConditions(t *testing.T) {
 
 		now = time.Now
 	})
+
+	//nolint:paralleltest // time.Now lives in global var `now` running this parallel will cause race cond
+	t.Run("test custom time shift", func(t *testing.T) {
+		identifier := &openIDIdentifier{
+			authCodeOptionerProvider: func() []AuthCodeOptioner { return nil },
+			oauth2Config:             &oauth2.Config{},
+			reverseRouter:            new(mockRouter),
+			responder:                &web.Responder{},
+		}
+
+		request, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://example.com/callback", nil)
+		assert.NoError(t, err)
+
+		session := web.EmptySession()
+
+		resp := identifier.Authenticate(context.Background(), web.CreateRequest(nil, session))
+		state1 := resp.(*web.URLRedirectResponse).URL.Query().Get("state")
+		oneHour := time.Hour
+		identifier.stateTimeout = &oneHour
+
+		now = func() time.Time {
+			return time.Now().Add(35 * time.Minute)
+		}
+
+		request.URL.RawQuery = url.Values{"state": []string{state1}}.Encode()
+		resp = identifier.Callback(context.Background(), web.CreateRequest(request, session), nil)
+		errResp := resp.(*web.ServerErrorResponse)
+		assert.NotContains(t, errResp.Error.Error(), "state mismatch")
+
+		now = time.Now
+	})
 }
 
 type testOidcProvider struct {
@@ -303,6 +349,8 @@ func TestOidcCallback(t *testing.T) {
 }
 
 func Test_openIDIdentifier_RefreshIdentity(t *testing.T) {
+	t.Parallel()
+
 	var identifier auth.RequestIdentifier = &openIDIdentifier{broker: "broker"}
 	session := web.EmptySession()
 	session.Store("core.auth.oidc.broker.sessiondata", sessionData{
-Original file line number
+Diff line change
@@ Expand Up / @@ -46,6 +46,7 @@ core: auth: { @@
     		}
     		enableEndSessionEndpoint: bool | *true
     		overrideIssuerURL: string | *""
+    		stateLifeTime: string | *"30m"
     	}
     }
     `
@@ Expand Down @@