[WIP] Refactor user service and co-relevant services to use transactions #1176
Conversation
There was a problem hiding this comment.
Pull request overview
This PR threads context.Context through OAuth2 / authn / notification / flow / authz services and handlers to support the new transaction-based architecture.
Changes:
- Updated backend service interfaces to accept
context.Contextand propagated request contexts from HTTP handlers. - Updated JWT/token-related utilities/builders/validators to pass context into downstream dependencies.
- Updated unit tests and generated mocks to match the new context-aware method signatures.
Reviewed changes
Copilot reviewed 85 out of 156 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/internal/oauth/oauth2/userinfo/userInfoServiceInterface_mock_test.go | Regenerated userinfo service mock to include context.Context in method signature. |
| backend/internal/oauth/oauth2/userinfo/service.go | Added context-aware GetUserInfo flow and propagated ctx to JWT/user lookups. |
| backend/internal/oauth/oauth2/userinfo/handler_test.go | Updated handler tests to expect ctx argument in mock calls. |
| backend/internal/oauth/oauth2/userinfo/handler.go | Passed r.Context() into GetUserInfo. |
| backend/internal/oauth/oauth2/tokenservice/validator.go | Added ctx to token validator methods and JWT verification calls. |
| backend/internal/oauth/oauth2/tokenservice/utils_test.go | Updated tokenservice utils tests to pass ctx and match ctx-aware user service calls. |
| backend/internal/oauth/oauth2/tokenservice/utils.go | Added ctx to FetchUserAttributesAndGroups and propagated to user service calls. |
| backend/internal/oauth/oauth2/tokenservice/builder.go | Updated token builder interface/impl to accept ctx and pass it into JWT generation. |
| backend/internal/oauth/oauth2/token/token_handler_test.go | Updated token handler tests for ctx-aware grant handler and refresh token issuance. |
| backend/internal/oauth/oauth2/token/token_handler.go | Passed r.Context() to grant validation/handling and refresh token issuance. |
| backend/internal/oauth/oauth2/introspect/service_test.go | Updated introspection service tests to pass ctx and match ctx-aware JWT verification mocks. |
| backend/internal/oauth/oauth2/introspect/service.go | Added ctx to introspection service and propagated to JWT verification. |
| backend/internal/oauth/oauth2/introspect/handler_test.go | Updated introspection handler tests to expect ctx argument in mock calls. |
| backend/internal/oauth/oauth2/introspect/handler.go | Passed r.Context() into IntrospectToken. |
| backend/internal/oauth/oauth2/introspect/TokenIntrospectionServiceInterface_mock_test.go | Regenerated introspection service mock to include context.Context. |
| backend/internal/oauth/oauth2/granthandlers/token_exchange.go | Added ctx to grant handler interface methods and propagated to validator/builder. |
| backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go | Updated refresh token grant tests to include ctx in calls/mocks. |
| backend/internal/oauth/oauth2/granthandlers/refresh_token.go | Added ctx to refresh token grant handler and propagated to validator/builder. |
| backend/internal/oauth/oauth2/granthandlers/grant_handler.go | Updated grant handler interfaces to accept ctx and refresh issuance to accept ctx. |
| backend/internal/oauth/oauth2/granthandlers/client_credentials_test.go | Updated client credentials tests for ctx-aware grant handler and builder calls. |
| backend/internal/oauth/oauth2/granthandlers/client_credentials.go | Added ctx to grant handling and propagated to token builder. |
| backend/internal/oauth/oauth2/granthandlers/authorization_code.go | Propagated ctx into user attribute/group fetch and token building. |
| backend/internal/oauth/oauth2/authz/handler_test.go | Updated authz handler tests to pass ctx and expect ctx-aware JWT verification. |
| backend/internal/oauth/oauth2/authz/handler.go | Propagated ctx through authorization response/assertion verification flow. |
| backend/internal/notification/otp_service_test.go | Updated OTP service tests to pass ctx and match ctx-aware JWT mocks. |
| backend/internal/notification/otp_service.go | Added ctx to OTP service interface and propagated to JWT generation/verification. |
| backend/internal/notification/message_handler_test.go | Updated notification handler tests for ctx-aware OTP service calls. |
| backend/internal/notification/message_handler.go | Passed r.Context() into OTP send/verify service calls. |
| backend/internal/notification/OTPServiceInterface_mock_test.go | Regenerated OTP service mock to include context.Context. |
| backend/internal/group/handler_test.go | Updated group handler tests to expect ctx argument in service methods. |
| backend/internal/group/handler.go | Passed r.Context() into group service calls. |
| backend/internal/flow/executor/sms_auth_executor.go | Propagated node context into user/OTP service calls in SMS OTP executor. |
| backend/internal/flow/executor/provisioning_executor.go | Propagated node context into user/group/role service calls and tweaked messages. |
| backend/internal/flow/executor/oidc_auth_executor.go | Added ctx-aware executor interface methods and propagated ctx to auth service calls. |
| backend/internal/flow/executor/oidcAuthExecutorInterface_mock_test.go | Updated executor mock for ctx-aware method signatures. |
| backend/internal/flow/executor/oauth_executor.go | Propagated node context into OAuth auth service calls and internal user resolution. |
| backend/internal/flow/executor/oAuthExecutorInterface_mock_test.go | Updated OAuth executor mock for ctx-aware internal user method signature. |
| backend/internal/flow/executor/identifying_executor_test.go | Updated identifying executor tests to pass ctx and match ctx-aware user service calls. |
| backend/internal/flow/executor/identifying_executor.go | Added ctx to IdentifyUser and propagated it to user service. |
| backend/internal/flow/executor/identifyingExecutorInterface_mock_test.go | Updated identifying executor mock for ctx-aware signature. |
| backend/internal/flow/executor/basic_auth_executor_test.go | Updated basic auth executor tests to expect ctx-aware user/creds calls. |
| backend/internal/flow/executor/basic_auth_executor.go | Propagated node context into IdentifyUser and credential authentication calls. |
| backend/internal/flow/executor/authz_executor.go | Propagated node context into authorization service call. |
| backend/internal/flow/executor/auth_assert_executor_test.go | Updated auth assertion tests for ctx-aware JWT generation and ctx-aware user service calls. |
| backend/internal/flow/executor/auth_assert_executor.go | Propagated ctx into JWT generation and user/OU lookups. |
| backend/internal/flow/executor/attribute_collector_test.go | Updated attribute collector tests for ctx-aware user service calls. |
| backend/internal/flow/executor/attribute_collector.go | Propagated node context into user retrieval/update calls. |
| backend/internal/authz/service_test.go | Updated authorization service tests to pass ctx and match ctx-aware engine calls. |
| backend/internal/authz/service.go | Added ctx to authorization service and propagated to engine. |
| backend/internal/authz/engine/rbacengine_test.go | Updated RBAC engine tests to pass ctx and match ctx-aware role service calls. |
| backend/internal/authz/engine/rbacengine.go | Added ctx to RBAC engine and propagated to role service. |
| backend/internal/authz/engine/engine.go | Updated authorization engine interface to accept ctx. |
| backend/internal/authn/passkey/service.go | Added ctx to passkey service and propagated to user service credential operations. |
| backend/internal/authn/otp/service_test.go | Updated OTP authn service tests to pass ctx and match ctx-aware notification/user calls. |
| backend/internal/authn/otp/service.go | Added ctx to OTP authn service methods and propagated to notification/user services. |
| backend/internal/authn/oidc/service.go | Added ctx to OIDC authn service core API and propagated to internal/jwt calls. |
| backend/internal/authn/oauth/service_test.go | Updated OAuth authn service tests to pass ctx and match ctx-aware service calls. |
| backend/internal/authn/oauth/service.go | Added ctx to OAuth authn service core API and propagated to user/idp operations. |
| backend/internal/authn/handler_test.go | Updated authn handler tests to include ctx parameter in mocked service calls. |
| backend/internal/authn/handler.go | Passed r.Context() into authn service calls (credentials/OTP/IDP/passkey). |
| backend/internal/authn/google/service.go | Added ctx to Google authn service methods and propagated to internal/jwt calls. |
| backend/internal/authn/github/service_test.go | Updated GitHub authn service tests to pass ctx and match ctx-aware OAuth service calls. |
| backend/internal/authn/github/service.go | Added ctx to GitHub authn service methods and propagated to internal OAuth calls. |
| backend/internal/authn/credentials/service_test.go | Updated credentials authn tests to pass ctx and match ctx-aware user service calls. |
| backend/internal/authn/credentials/service.go | Added ctx to credentials authn service and propagated to user service calls. |
|
|
||
| func (s *UserInfoHandlerTestSuite) SetupTest() { | ||
| s.mockService = new(userInfoServiceInterfaceMock) | ||
| s.mockService = new(UserInfoServiceInterfaceMock) |
There was a problem hiding this comment.
The test suite initializes the mock with new(UserInfoServiceInterfaceMock), which bypasses the generated NewUserInfoServiceInterfaceMock(t) helper that registers mock.Mock.Test(t) and a Cleanup assertion for expectations. Consider switching to the helper (e.g., s.mockService = NewUserInfoServiceInterfaceMock(s.T())) so unmet expectations are reliably reported.
| s.mockService = new(UserInfoServiceInterfaceMock) | |
| s.mockService = NewUserInfoServiceInterfaceMock(s.T()) |
| attributesJSON, err := json.Marshal(userAttributes) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal user attributes: %w", err) | ||
| return nil, fmt.Errorf("failed to marshal attributes: %w", err) |
There was a problem hiding this comment.
This error message became less specific after the refactor. Since the marshaling here is specifically for user attributes, keeping the original context (e.g., 'failed to marshal user attributes') would make logs and debugging clearer.
| return nil, fmt.Errorf("failed to marshal attributes: %w", err) | |
| return nil, fmt.Errorf("failed to marshal user attributes: %w", err) |
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
224889f to
2b0818d
Compare
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
2b0818d to
1661dfc
Compare
| // Start passkey authentication | ||
| // TODO: Remove context.TODO() when context is available |
There was a problem hiding this comment.
These calls use context.TODO() even though this executor already receives a *core.NodeContext (and other executors in this PR propagate ctx.Context). This breaks cancellation/deadline propagation and may bypass transaction scoping. Prefer passing the existing node/request context (e.g., ctx.Context) rather than context.TODO().
| RelyingPartyID: relyingPartyID, | ||
| } | ||
| startData, svcErr := p.passkeyService.StartAuthentication(startReq) | ||
| startData, svcErr := p.passkeyService.StartAuthentication(context.TODO(), startReq) |
There was a problem hiding this comment.
These calls use context.TODO() even though this executor already receives a *core.NodeContext (and other executors in this PR propagate ctx.Context). This breaks cancellation/deadline propagation and may bypass transaction scoping. Prefer passing the existing node/request context (e.g., ctx.Context) rather than context.TODO().
| startData, svcErr := p.passkeyService.StartAuthentication(context.TODO(), startReq) | |
| startData, svcErr := p.passkeyService.StartAuthentication(ctx.Context, startReq) |
| SessionToken: sessionToken, | ||
| } | ||
| authResp, svcErr := p.passkeyService.FinishAuthentication(finishReq) | ||
| authResp, svcErr := p.passkeyService.FinishAuthentication(context.TODO(), finishReq) |
There was a problem hiding this comment.
These calls use context.TODO() even though this executor already receives a *core.NodeContext (and other executors in this PR propagate ctx.Context). This breaks cancellation/deadline propagation and may bypass transaction scoping. Prefer passing the existing node/request context (e.g., ctx.Context) rather than context.TODO().
| authResp, svcErr := p.passkeyService.FinishAuthentication(context.TODO(), finishReq) | |
| authResp, svcErr := p.passkeyService.FinishAuthentication(ctx.Context, finishReq) |
|
|
||
| // Get user details from user service | ||
| user, svcErr := p.userService.GetUser(userID) | ||
| user, svcErr := p.userService.GetUser(context.TODO(), userID) |
There was a problem hiding this comment.
These calls use context.TODO() even though this executor already receives a *core.NodeContext (and other executors in this PR propagate ctx.Context). This breaks cancellation/deadline propagation and may bypass transaction scoping. Prefer passing the existing node/request context (e.g., ctx.Context) rather than context.TODO().
| user, svcErr := p.userService.GetUser(context.TODO(), userID) | |
| user, svcErr := p.userService.GetUser(ctx.Context, userID) |
|
|
||
| // Start passkey registration | ||
| startData, svcErr := p.passkeyService.StartRegistration(regReq) | ||
| startData, svcErr := p.passkeyService.StartRegistration(context.TODO(), regReq) |
There was a problem hiding this comment.
These calls use context.TODO() even though this executor already receives a *core.NodeContext (and other executors in this PR propagate ctx.Context). This breaks cancellation/deadline propagation and may bypass transaction scoping. Prefer passing the existing node/request context (e.g., ctx.Context) rather than context.TODO().
| startData, svcErr := p.passkeyService.StartRegistration(context.TODO(), regReq) | |
| startData, svcErr := p.passkeyService.StartRegistration(ctx.Context, regReq) |
|
|
||
| // Call passkey service to finish registration | ||
| finishData, svcErr := p.passkeyService.FinishRegistration(finishReq) | ||
| finishData, svcErr := p.passkeyService.FinishRegistration(context.TODO(), finishReq) |
There was a problem hiding this comment.
These calls use context.TODO() even though this executor already receives a *core.NodeContext (and other executors in this PR propagate ctx.Context). This breaks cancellation/deadline propagation and may bypass transaction scoping. Prefer passing the existing node/request context (e.g., ctx.Context) rather than context.TODO().
| // Try to identify the user | ||
| // TODO: Remove context.TODO() when context is available | ||
| filters := map[string]interface{}{userAttributeUsername: username} | ||
| userID, err := i.IdentifyUser(filters, execResp) | ||
| userID, err := i.IdentifyUser(context.TODO(), filters, execResp) |
There was a problem hiding this comment.
Similar to other flow executors updated in this PR, this should propagate the existing execution/request context instead of using context.TODO(). Using TODO here undermines the goal of making services transaction/context aware.
| // TODO: Remove context.TODO() when context is available | ||
| svcErr := e.userService.UpdateUserCredentials(context.TODO(), userID, credentials) |
There was a problem hiding this comment.
This is another context.TODO() usage in a request-driven code path. Since the executor already has access to ctx *core.NodeContext, it should pass through the real context (e.g., ctx.Context) so downstream services can participate in cancellation/transactions.
| if err := s.jwtService.VerifyJWT(token, "", ""); err != nil { | ||
| func (s *tokenIntrospectionService) validateToken(ctx context.Context, logger *log.Logger, token string) bool { | ||
| if err := s.jwtService.VerifyJWT(ctx, token, "", ""); err != nil { | ||
| logger.Debug("Failed to verify refresh token", log.String("error", err.Error)) |
There was a problem hiding this comment.
The log message says "Failed to verify refresh token" but this code validates the introspected token (which can be access token, refresh token, etc.). This looks like a copy/paste message and makes troubleshooting misleading. Update the message to refer to the generic token being introspected.
| logger.Debug("Failed to verify refresh token", log.String("error", err.Error)) | |
| logger.Debug("Failed to verify token", log.String("error", err.Error)) |
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
1661dfc to
7970db5
Compare
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
7970db5 to
7bdb515
Compare
|
|
||
| // userInfoServiceInterfaceMock is an autogenerated mock type for the userInfoServiceInterface type | ||
| type userInfoServiceInterfaceMock struct { | ||
| // UserInfoServiceInterfaceMock is an autogenerated mock type for the UserInfoServiceInterface type |
There was a problem hiding this comment.
This comment references UserInfoServiceInterface, but the actual interface in service.go is named userInfoServiceInterface (unexported). Update the comment to match the real interface name (or align the interface/type naming consistently) to avoid confusion when navigating generated mocks.
| // UserInfoServiceInterfaceMock is an autogenerated mock type for the UserInfoServiceInterface type | |
| // UserInfoServiceInterfaceMock is an autogenerated mock type for the userInfoServiceInterface type |
| RelyingPartyID: relyingPartyID, | ||
| } | ||
| startData, svcErr := p.passkeyService.StartAuthentication(startReq) | ||
| startData, svcErr := p.passkeyService.StartAuthentication(context.TODO(), startReq) |
There was a problem hiding this comment.
In this method you already have access to the flow/request context via the *core.NodeContext parameter. Using context.TODO() prevents cancellation, deadlines, and transaction-scoped values (likely the purpose of this refactor) from flowing into the passkey service. Pass ctx.Context instead and remove the TODO comment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1176 +/- ##
===========================================
- Coverage 88.72% 77.86% -10.87%
===========================================
Files 586 587 +1
Lines 39058 39477 +419
Branches 1998 1995 -3
===========================================
- Hits 34653 30737 -3916
- Misses 2629 5783 +3154
- Partials 1776 2957 +1181
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
7bdb515 to
28cd5be
Compare
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
28cd5be to
6c5b882
Compare
| func (tv *tokenValidator) verifyTokenSignatureByIssuer( | ||
| ctx context.Context, | ||
| token string, | ||
| issuer string, | ||
| oauthApp *appmodel.OAuthAppConfigProcessedDTO, | ||
| ) error { | ||
| issuers := getValidIssuers(oauthApp) | ||
| if issuers[issuer] { | ||
| svcErr := tv.jwtService.VerifyJWTSignature(token) | ||
| svcErr := tv.jwtService.VerifyJWTSignature(ctx, token) | ||
| if svcErr != nil { | ||
| return fmt.Errorf("failed to verify token signature: %v", svcErr) | ||
| } |
There was a problem hiding this comment.
If issuer is not in the allow-list (issuers[issuer] is false), the function returns nil without verifying the subject token signature, effectively allowing untrusted issuers to bypass signature verification. Consider explicitly rejecting unknown issuers (e.g., return an error when issuer is not allowed) so every accepted token has its signature verified.
| if err := s.jwtService.VerifyJWT(token, "", ""); err != nil { | ||
| func (s *tokenIntrospectionService) validateToken(ctx context.Context, logger *log.Logger, token string) bool { | ||
| if err := s.jwtService.VerifyJWT(ctx, token, "", ""); err != nil { | ||
| logger.Debug("Failed to verify refresh token", log.String("error", err.Error)) |
There was a problem hiding this comment.
This method validates any introspected token, but the log message says "refresh token". Consider changing it to a neutral message like "Failed to verify token" to avoid misleading debugging output.
| logger.Debug("Failed to verify refresh token", log.String("error", err.Error)) | |
| logger.Debug("Failed to verify token", log.String("error", err.Error())) |
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
6c5b882 to
41edaf1
Compare
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
41edaf1 to
33d8881
Compare
| // TODO: Remove context.TODO() when context is available | ||
| startReq := &passkey.PasskeyAuthenticationStartRequest{ | ||
| UserID: userID, | ||
| RelyingPartyID: relyingPartyID, | ||
| } | ||
| startData, svcErr := p.passkeyService.StartAuthentication(startReq) | ||
| startData, svcErr := p.passkeyService.StartAuthentication(context.TODO(), startReq) |
There was a problem hiding this comment.
The executor already receives ctx *core.NodeContext, so ctx.Context is available here. Using context.TODO() drops cancellation/deadline/trace propagation. Pass ctx.Context instead (and remove the TODO comment).
| // Try to identify the user | ||
| // TODO: Remove context.TODO() when context is available | ||
| filters := map[string]interface{}{userAttributeUsername: username} | ||
| userID, err := i.IdentifyUser(filters, execResp) | ||
| userID, err := i.IdentifyUser(context.TODO(), filters, execResp) |
There was a problem hiding this comment.
Execute already has access to ctx *core.NodeContext, so ctx.Context can be used instead of context.TODO() to preserve request-scoped cancellation and tracing.
| // TODO: Remove context.TODO() when context is available | ||
| svcErr := e.userService.UpdateUserCredentials(context.TODO(), userID, credentials) |
There was a problem hiding this comment.
Since this executor has ctx *core.NodeContext, it should pass ctx.Context into UpdateUserCredentials instead of context.TODO() to keep context propagation consistent across the flow.
| // BuildAuthorizeURL constructs the authorization request URL for Google OIDC authentication. | ||
| func (g *googleOIDCAuthnService) BuildAuthorizeURL(idpID string) (string, *serviceerror.ServiceError) { | ||
| return g.internal.BuildAuthorizeURL(idpID) | ||
| // BuildAuthorizeURL constructs the authorization request URL for Google OIDC authentication. |
There was a problem hiding this comment.
Duplicate comment line (same sentence appears twice). Remove one to keep docs clean.
| // BuildAuthorizeURL constructs the authorization request URL for Google OIDC authentication. |
| "github.com/stretchr/testify/mock" | ||
| "testing" | ||
|
|
There was a problem hiding this comment.
The import block isn’t gofmt-compliant (missing standard/third-party grouping and ordering). Running gofmt would reorder/group these imports correctly (e.g., testing with stdlib, then third-party packages in a separate group).
| "github.com/stretchr/testify/mock" | |
| "testing" | |
| "testing" | |
| "github.com/stretchr/testify/mock" |
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
33d8881 to
9afb707
Compare
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
9afb707 to
6622e62
Compare
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
6622e62 to
41edaf1
Compare
JeethJJ
force-pushed
the
db-transaction-mgt
branch
from
41edaf1 to
6622e62
Compare
| } | ||
|
|
||
| // Start passkey authentication | ||
| // TODO: Remove context.TODO() when context is available |
There was a problem hiding this comment.
passkey_executor.go is still using context.TODO() even though a request/node context is available in this execution path. With the new transaction architecture, using context.TODO() will drop any request-scoped transaction/cancellation/deadlines and can cause operations to run outside the intended transaction. Pass ctx.Context (from *core.NodeContext) into these service calls instead of context.TODO() (same applies to the other context.TODO() passkey/user service calls in this file).
JeethJJ
changed the title
1 participant
Purpose
As we have introduced the transection architecture, we are adapting our services to use transections starting with user service. In this PR we have updated the user service and co-relevant services to use transactions.
Sub Issue : #1107
Parent issue : #282