From b35b62fed16e24ada19f182a3ccb205a743293a8 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 28 Oct 2024 14:21:22 +0100 Subject: [PATCH 1/6] feat: add identity ID to password grant extra claims --- handler/oauth2/flow_resource_owner.go | 6 +++++- handler/oauth2/flow_resource_owner_storage.go | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/handler/oauth2/flow_resource_owner.go b/handler/oauth2/flow_resource_owner.go index ad0b45a2..ff17afb0 100644 --- a/handler/oauth2/flow_resource_owner.go +++ b/handler/oauth2/flow_resource_owner.go @@ -58,10 +58,14 @@ func (c *ResourceOwnerPasswordCredentialsGrantHandler) HandleTokenEndpointReques password := request.GetRequestForm().Get("password") if username == "" || password == "" { return errorsx.WithStack(fosite.ErrInvalidRequest.WithHint("Username or password are missing from the POST body.")) - } else if err := c.ResourceOwnerPasswordCredentialsGrantStorage.Authenticate(ctx, username, password); errors.Is(err, fosite.ErrNotFound) { + } else if identityID, err := c.ResourceOwnerPasswordCredentialsGrantStorage.Authenticate(ctx, username, password); errors.Is(err, fosite.ErrNotFound) { return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint("Unable to authenticate the provided username and password credentials.").WithWrap(err).WithDebug(err.Error())) } else if err != nil { return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error())) + } else { + if sess, ok := request.GetSession().(fosite.ExtraClaimsSession); ok { + sess.GetExtraClaims()["identity_id"] = identityID + } } // Credentials must not be passed around, potentially leaking to the database! diff --git a/handler/oauth2/flow_resource_owner_storage.go b/handler/oauth2/flow_resource_owner_storage.go index ffc07642..d70a0467 100644 --- a/handler/oauth2/flow_resource_owner_storage.go +++ b/handler/oauth2/flow_resource_owner_storage.go @@ -8,7 +8,7 @@ import ( ) type ResourceOwnerPasswordCredentialsGrantStorage interface { - Authenticate(ctx context.Context, name string, secret string) error + Authenticate(ctx context.Context, name string, secret string) (string, error) AccessTokenStorage RefreshTokenStorage } From 199f8ab66014076328cee95b6b9f9e4aa73e631c Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 28 Oct 2024 14:54:52 +0100 Subject: [PATCH 2/6] update mock --- internal/oauth2_owner_storage.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/internal/oauth2_owner_storage.go b/internal/oauth2_owner_storage.go index 79e79795..114d2dbb 100644 --- a/internal/oauth2_owner_storage.go +++ b/internal/oauth2_owner_storage.go @@ -1,6 +1,3 @@ -// Copyright © 2024 Ory Corp -// SPDX-License-Identifier: Apache-2.0 - // Code generated by MockGen. DO NOT EDIT. // Source: github.com/ory/fosite/handler/oauth2 (interfaces: ResourceOwnerPasswordCredentialsGrantStorage) @@ -12,7 +9,6 @@ import ( reflect "reflect" gomock "github.com/golang/mock/gomock" - fosite "github.com/ory/fosite" ) @@ -40,11 +36,12 @@ func (m *MockResourceOwnerPasswordCredentialsGrantStorage) EXPECT() *MockResourc } // Authenticate mocks base method. -func (m *MockResourceOwnerPasswordCredentialsGrantStorage) Authenticate(arg0 context.Context, arg1, arg2 string) error { +func (m *MockResourceOwnerPasswordCredentialsGrantStorage) Authenticate(arg0 context.Context, arg1, arg2 string) (string, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Authenticate", arg0, arg1, arg2) - ret0, _ := ret[0].(error) - return ret0 + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 } // Authenticate indicates an expected call of Authenticate. From 7f6e7908e3494e9689a7d465aed74d43564cc14d Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 29 Oct 2024 12:01:51 +0100 Subject: [PATCH 3/6] code review --- handler/oauth2/flow_resource_owner.go | 11 ++++++++--- handler/oauth2/flow_resource_owner_storage.go | 2 +- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/handler/oauth2/flow_resource_owner.go b/handler/oauth2/flow_resource_owner.go index ff17afb0..8c1e0437 100644 --- a/handler/oauth2/flow_resource_owner.go +++ b/handler/oauth2/flow_resource_owner.go @@ -33,6 +33,11 @@ type ResourceOwnerPasswordCredentialsGrantHandler struct { } } +type Session interface { + // SetSubject sets the session's subject. + SetSubject(subject string) +} + // HandleTokenEndpointRequest implements https://tools.ietf.org/html/rfc6749#section-4.3.2 func (c *ResourceOwnerPasswordCredentialsGrantHandler) HandleTokenEndpointRequest(ctx context.Context, request fosite.AccessRequester) error { if !c.CanHandleTokenEndpointRequest(ctx, request) { @@ -58,13 +63,13 @@ func (c *ResourceOwnerPasswordCredentialsGrantHandler) HandleTokenEndpointReques password := request.GetRequestForm().Get("password") if username == "" || password == "" { return errorsx.WithStack(fosite.ErrInvalidRequest.WithHint("Username or password are missing from the POST body.")) - } else if identityID, err := c.ResourceOwnerPasswordCredentialsGrantStorage.Authenticate(ctx, username, password); errors.Is(err, fosite.ErrNotFound) { + } else if sub, err := c.ResourceOwnerPasswordCredentialsGrantStorage.Authenticate(ctx, username, password); errors.Is(err, fosite.ErrNotFound) { return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint("Unable to authenticate the provided username and password credentials.").WithWrap(err).WithDebug(err.Error())) } else if err != nil { return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error())) } else { - if sess, ok := request.GetSession().(fosite.ExtraClaimsSession); ok { - sess.GetExtraClaims()["identity_id"] = identityID + if sess, ok := request.GetSession().(Session); ok { + sess.SetSubject(sub) } } diff --git a/handler/oauth2/flow_resource_owner_storage.go b/handler/oauth2/flow_resource_owner_storage.go index d70a0467..b0b4efdb 100644 --- a/handler/oauth2/flow_resource_owner_storage.go +++ b/handler/oauth2/flow_resource_owner_storage.go @@ -8,7 +8,7 @@ import ( ) type ResourceOwnerPasswordCredentialsGrantStorage interface { - Authenticate(ctx context.Context, name string, secret string) (string, error) + Authenticate(ctx context.Context, name string, secret string) (subject string, err error) AccessTokenStorage RefreshTokenStorage } From 04c14cdb4bba1e2c4199d6e35c88a30ef609a230 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 29 Oct 2024 12:06:27 +0100 Subject: [PATCH 4/6] chore: format --- internal/oauth2_owner_storage.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/oauth2_owner_storage.go b/internal/oauth2_owner_storage.go index 114d2dbb..7e79b178 100644 --- a/internal/oauth2_owner_storage.go +++ b/internal/oauth2_owner_storage.go @@ -1,3 +1,6 @@ +// Copyright © 2024 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + // Code generated by MockGen. DO NOT EDIT. // Source: github.com/ory/fosite/handler/oauth2 (interfaces: ResourceOwnerPasswordCredentialsGrantStorage) From e776a3f7de75c6dffa5659b670fbf00834d27869 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 29 Oct 2024 12:22:37 +0100 Subject: [PATCH 5/6] fix relevant tests --- handler/oauth2/flow_resource_owner_test.go | 6 +++--- storage/memory.go | 9 +++++---- storage/memory_test.go | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/handler/oauth2/flow_resource_owner_test.go b/handler/oauth2/flow_resource_owner_test.go index 59acb5ab..6e8280ac 100644 --- a/handler/oauth2/flow_resource_owner_test.go +++ b/handler/oauth2/flow_resource_owner_test.go @@ -71,21 +71,21 @@ func TestResourceOwnerFlow_HandleTokenEndpointRequest(t *testing.T) { areq.Form.Set("password", "pan") areq.Client = &fosite.DefaultClient{GrantTypes: fosite.Arguments{"password"}, Scopes: []string{"foo-scope"}, Audience: []string{"https://www.ory.sh/api"}} - store.EXPECT().Authenticate(gomock.Any(), "peter", "pan").Return(fosite.ErrNotFound) + store.EXPECT().Authenticate(gomock.Any(), "peter", "pan").Return("", fosite.ErrNotFound) }, expectErr: fosite.ErrInvalidGrant, }, { description: "should fail because error on lookup", setup: func(config *fosite.Config) { - store.EXPECT().Authenticate(gomock.Any(), "peter", "pan").Return(errors.New("")) + store.EXPECT().Authenticate(gomock.Any(), "peter", "pan").Return("", errors.New("")) }, expectErr: fosite.ErrServerError, }, { description: "should pass", setup: func(config *fosite.Config) { - store.EXPECT().Authenticate(gomock.Any(), "peter", "pan").Return(nil) + store.EXPECT().Authenticate(gomock.Any(), "peter", "pan").Return("", nil) }, check: func(areq *fosite.AccessRequest) { //assert.NotEmpty(t, areq.GetSession().GetExpiresAt(fosite.AccessToken)) diff --git a/storage/memory.go b/storage/memory.go index 9e9fd1ba..5e88772a 100644 --- a/storage/memory.go +++ b/storage/memory.go @@ -10,6 +10,7 @@ import ( "time" "github.com/go-jose/go-jose/v3" + "github.com/google/uuid" "github.com/ory/fosite" "github.com/ory/fosite/internal" @@ -355,18 +356,18 @@ func (s *MemoryStore) DeleteRefreshTokenSession(_ context.Context, signature str return nil } -func (s *MemoryStore) Authenticate(_ context.Context, name string, secret string) error { +func (s *MemoryStore) Authenticate(_ context.Context, name string, secret string) (subject string, err error) { s.usersMutex.RLock() defer s.usersMutex.RUnlock() rel, ok := s.Users[name] if !ok { - return fosite.ErrNotFound + return "", fosite.ErrNotFound } if rel.Password != secret { - return fosite.ErrNotFound.WithDebug("Invalid credentials") + return "", fosite.ErrNotFound.WithDebug("Invalid credentials") } - return nil + return uuid.New().String(), nil } func (s *MemoryStore) RevokeRefreshToken(ctx context.Context, requestID string) error { diff --git a/storage/memory_test.go b/storage/memory_test.go index 0f082ee8..d06f4a40 100644 --- a/storage/memory_test.go +++ b/storage/memory_test.go @@ -52,7 +52,7 @@ func TestMemoryStore_Authenticate(t *testing.T) { Users: tt.fields.Users, usersMutex: tt.fields.usersMutex, } - if err := s.Authenticate(tt.args.in0, tt.args.name, tt.args.secret); err == nil || !errors.Is(err, tt.wantErr) { + if _, err := s.Authenticate(tt.args.in0, tt.args.name, tt.args.secret); err == nil || !errors.Is(err, tt.wantErr) { t.Errorf("Authenticate() error = %v, wantErr %v", err, tt.wantErr) } }) From 62f07ce22e575480fde42f3468d827786eed3c4d Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 29 Oct 2024 12:24:24 +0100 Subject: [PATCH 6/6] overwrite hydra for oidc tests --- .github/workflows/oidc-conformity.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/oidc-conformity.yml b/.github/workflows/oidc-conformity.yml index 1c7ecdd0..6994fb5e 100644 --- a/.github/workflows/oidc-conformity.yml +++ b/.github/workflows/oidc-conformity.yml @@ -14,7 +14,7 @@ jobs: with: fetch-depth: 2 repository: ory/hydra - ref: master + ref: 2866a0499d02341ed0603601cfe4e63b24506fcb - uses: actions/setup-go@v2 with: go-version: "1.21"