fix(openid): session not deleted after final use (#44)

This fixes an issue where the OpenID Connect 1.0 session was not deleted after it being used.
authelia · Feb 14, 2024 · 5f39274 · 5f39274
1 parent c835349
commit 5f39274
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 3 deletions.
diff --git a/handler/openid/flow_explicit_token.go b/handler/openid/flow_explicit_token.go
@@ -22,7 +22,9 @@ func (c *OpenIDConnectExplicitHandler) PopulateTokenEndpointResponse(ctx context
 		return errorsx.WithStack(oauth2.ErrUnknownRequest)
 	}
 
-	authorize, err := c.OpenIDConnectRequestStorage.GetOpenIDConnectSession(ctx, requester.GetRequestForm().Get(consts.FormParameterAuthorizationCode), requester)
+	authorizeCode := requester.GetRequestForm().Get(consts.FormParameterAuthorizationCode)
+
+	authorize, err := c.OpenIDConnectRequestStorage.GetOpenIDConnectSession(ctx, authorizeCode, requester)
 	if errors.Is(err, ErrNoSessionFound) {
 		return errorsx.WithStack(oauth2.ErrUnknownRequest.WithWrap(err).WithDebugError(err))
 	} else if err != nil {
@@ -42,6 +44,10 @@ func (c *OpenIDConnectExplicitHandler) PopulateTokenEndpointResponse(ctx context
 		return errorsx.WithStack(oauth2.ErrServerError.WithDebug("Failed to generate id token because session must be of type oauth2/handler/openid.Session."))
 	}
 
+	if err = c.OpenIDConnectRequestStorage.DeleteOpenIDConnectSession(ctx, authorizeCode); err != nil {
+		return errorsx.WithStack(oauth2.ErrServerError.WithWrap(err).WithDebugError(err))
+	}
+
 	claims := sess.IDTokenClaims()
 	if claims.Subject == "" {
 		return errorsx.WithStack(oauth2.ErrServerError.WithDebug("Failed to generate id token because subject is an empty string."))

diff --git a/handler/openid/flow_explicit_token_test.go b/handler/openid/flow_explicit_token_test.go
@@ -103,6 +103,7 @@ func TestExplicit_PopulateTokenEndpointResponse(t *testing.T) {
 				storedReq.GrantedScope = oauth2.Arguments{consts.ScopeOpenID}
 				storedReq.Form.Set(consts.FormParameterNonce, "1111111111111111")
 				store.EXPECT().GetOpenIDConnectSession(context.TODO(), "foobar", req).Return(storedReq, nil)
+				store.EXPECT().DeleteOpenIDConnectSession(gomock.Any(), "foobar").Return(nil)
 			},
 			check: func(t *testing.T, aresp *oauth2.AccessResponse) {
 				assert.NotEmpty(t, aresp.GetExtra(consts.AccessResponseIDToken))
@@ -133,6 +134,7 @@ func TestExplicit_PopulateTokenEndpointResponse(t *testing.T) {
 				storedReq.GrantedScope = oauth2.Arguments{consts.ScopeOpenID}
 				storedReq.Form.Set("nonce", "1111111111111111")
 				store.EXPECT().GetOpenIDConnectSession(context.TODO(), "foobar", req).Return(storedReq, nil)
+				store.EXPECT().DeleteOpenIDConnectSession(gomock.Any(), "foobar").Return(nil)
 			},
 			check: func(t *testing.T, aresp *oauth2.AccessResponse) {
 				assert.NotEmpty(t, aresp.GetExtra("id_token"))
@@ -159,6 +161,7 @@ func TestExplicit_PopulateTokenEndpointResponse(t *testing.T) {
 				storedReq.Session = storedSession
 				storedReq.GrantedScope = oauth2.Arguments{consts.ScopeOpenID}
 				store.EXPECT().GetOpenIDConnectSession(context.TODO(), "foobar", req).Return(storedReq, nil)
+				store.EXPECT().DeleteOpenIDConnectSession(gomock.Any(), "foobar").Return(nil)
 			},
 			expectErr: oauth2.ErrServerError,
 		},
@@ -174,6 +177,25 @@ func TestExplicit_PopulateTokenEndpointResponse(t *testing.T) {
 			},
 			expectErr: oauth2.ErrServerError,
 		},
+		{
+			description: "should fail because storage returns error when deleting openid session",
+			setup: func(store *internal.MockOpenIDConnectRequestStorage, req *oauth2.AccessRequest) {
+				req.Client = &oauth2.DefaultClient{
+					GrantTypes: oauth2.Arguments{"authorization_code"},
+				}
+				req.GrantTypes = oauth2.Arguments{"authorization_code"}
+				req.Form.Set("code", "foobar")
+				storedSession := &DefaultSession{
+					Claims: &jwt.IDTokenClaims{Subject: "peter"},
+				}
+				storedReq := oauth2.NewAuthorizeRequest()
+				storedReq.Session = storedSession
+				storedReq.GrantedScope = oauth2.Arguments{"openid"}
+				store.EXPECT().GetOpenIDConnectSession(gomock.Any(), "foobar", req).Return(storedReq, nil)
+				store.EXPECT().DeleteOpenIDConnectSession(gomock.Any(), "foobar").Return(errors.New("delete openid session err"))
+			},
+			expectErr: oauth2.ErrServerError,
+		},
 	} {
 		t.Run(fmt.Sprintf("case=%d/description=%s", k, c.description), func(t *testing.T) {
 			ctrl := gomock.NewController(t)

diff --git a/handler/openid/storage.go b/handler/openid/storage.go
@@ -22,7 +22,6 @@ type OpenIDConnectRequestStorage interface {
 	// - or an arbitrary error if an error occurred.
 	GetOpenIDConnectSession(ctx context.Context, authorizeCode string, requester oauth2.Requester) (oauth2.Requester, error)
 
-	// Deprecated: DeleteOpenIDConnectSession is not called from anywhere.
-	// Originally, it should remove an open id connect session from the store.
+	// DeleteOpenIDConnectSession deletes the OpenID Connect 1.0 session from storage.
 	DeleteOpenIDConnectSession(ctx context.Context, authorizeCode string) error
 }