Refactor authentication services to use di pattern#673
Refactor authentication services to use di pattern#673ThaminduDilshan merged 1 commit intoasgardeo:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements dependency injection pattern for authentication services across the backend, replacing direct instantiation with constructor-based injection. The changes introduce proper service initialization functions, deprecate the old "New" constructors, and update service registration in the main application bootstrap.
- Refactored authentication services to use dependency injection pattern with private
new*constructors and publicInitialize()functions - Updated service wiring in
servicemanager.goto properly inject dependencies between authentication services - Added mock setup for
AuthAssertGeneratorInterfacein tests to ensure proper test coverage of the new pattern
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/tests/mocks/authn/oidcmock/OIDCAuthnCoreServiceInterface_mock.go | Added GetOAuthClientConfig mock method to support new interface requirements |
| backend/tests/mocks/authn/oauthmock/OAuthAuthnCoreServiceInterface_mock.go | Added GetOAuthClientConfig mock method to OAuth authenticator mock |
| backend/tests/mocks/authn/googlemock/GoogleOIDCAuthnServiceInterface_mock.go | Added GetOAuthClientConfig mock method to Google OIDC authenticator mock |
| backend/tests/mocks/authn/githubmock/GithubOAuthAuthnServiceInterface_mock.go | Added GetOAuthClientConfig mock method to GitHub OAuth authenticator mock |
| backend/internal/notification/init.go | Updated to return both NotificationSenderMgtSvcInterface and OTPServiceInterface for proper dependency injection |
| backend/internal/executor/oidcauth/oidcauthexecutor.go | Refactored to directly instantiate dependencies instead of nesting service constructors |
| backend/internal/executor/oauth/oauthexecutor.go | Refactored OAuth executor to use flattened dependency instantiation |
| backend/internal/executor/googleauth/googleauthexecutor.go | Simplified Google auth executor by removing nested service creation and using direct dependency injection |
| backend/internal/executor/githubauth/githubauthexecutor.go | Refactored GitHub executor to use direct service instantiation with proper dependencies |
| backend/internal/authn/service_test.go | Added mock for AuthAssertGeneratorInterface and updated test setup to use struct literal initialization |
| backend/internal/authn/service.go | Changed NewAuthenticationService to private newAuthenticationService with explicit dependency parameters |
| backend/internal/authn/otp/service_test.go | Updated tests to use private newOTPAuthnService constructor |
| backend/internal/authn/otp/service.go | Added private newOTPAuthnService constructor and deprecated public NewOTPAuthnService |
| backend/internal/authn/otp/init.go | Added Initialize() function to create OTP authentication service with injected dependencies |
| backend/internal/authn/oidc/service_test.go | Updated tests to use new constructor pattern with all required dependencies |
| backend/internal/authn/oidc/service.go | Refactored to use private newOIDCAuthnService with full dependency injection and deprecated old constructor |
| backend/internal/authn/oidc/init.go | Added Initialize() function for OIDC authentication service |
| backend/internal/authn/oauth/service.go | Changed to use private newOAuthAuthnService constructor with explicit dependencies and deprecated old constructor |
| backend/internal/authn/oauth/init.go | Added Initialize() function for OAuth authentication service |
| backend/internal/authn/init.go | Transformed from service wrapper to proper initialization function with dependency injection |
| backend/internal/authn/handler_test.go | Updated to reference private authenticationHandler type |
| backend/internal/authn/handler.go | Changed handler to use private type and constructor accepting injected service |
| backend/internal/authn/google/service.go | Refactored Google OIDC service to use private constructor with dependencies and added GetOAuthClientConfig method |
| backend/internal/authn/google/init.go | Added Initialize() function for Google OIDC authentication service |
| backend/internal/authn/github/service.go | Refactored GitHub OAuth service with private constructor and added GetOAuthClientConfig method |
| backend/internal/authn/github/init.go | Added Initialize() function for GitHub OAuth authentication service |
| backend/internal/authn/credentials/service_test.go | Updated tests to use private newCredentialsAuthnService constructor |
| backend/internal/authn/credentials/service.go | Added private newCredentialsAuthnService and deprecated public constructor |
| backend/internal/authn/credentials/init.go | Added Initialize() function for credentials authentication service |
| backend/internal/authn/assert/init.go | Added Initialize() function for auth assert generator |
| backend/internal/authn/assert/generator_test.go | Updated test to use private constructor |
| backend/internal/authn/assert/generator.go | Added private newAuthAssertGenerator constructor and deprecated public one |
| backend/cmd/server/servicemanager.go | Updated service registration to use new dependency injection pattern with proper ordering |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #673 +/- ##
=======================================
Coverage ? 69.52%
=======================================
Files ? 229
Lines ? 20179
Branches ? 262
=======================================
Hits ? 14030
Misses ? 4684
Partials ? 1465
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:
|
5a5d1ed to
2110115
Compare
2110115 to
22de2d5
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
backend/internal/authn/init.go:97
- Inconsistent handler usage: This OPTIONS handler uses an inline anonymous function while other OPTIONS handlers in the same function use
optionsNoContentHandler. For consistency and maintainability, useoptionsNoContentHandlerhere as well.
backend/internal/authn/init.go:109 - Inconsistent handler usage: This OPTIONS handler uses an inline anonymous function while other OPTIONS handlers in the same function use
optionsNoContentHandler. For consistency and maintainability, useoptionsNoContentHandlerhere as well.
backend/internal/authn/init.go:121 - Inconsistent handler usage: This OPTIONS handler uses an inline anonymous function while other OPTIONS handlers in the same function use
optionsNoContentHandler. For consistency and maintainability, useoptionsNoContentHandlerhere as well.
| idpService, jwtService, | ||
| assertGenerator, credentialsAuthnService, otpAuthnService, | ||
| oAuthAuthnService, oidcAuthnService, googleAuthnService, githubAuthnService, | ||
| ) |
There was a problem hiding this comment.
Rather than creating all these Service in the servicemanager level, can't we do this
In the servicemanager.go, we will Initialize the authentication service with all the needed services
# servicemanager.go
authnService, executorProvider = authn.Initialize(
mux,, userService
idpService, jwtService
)
....
_ := flowExec.Initialize(mux, executorProvider)
And we will have a authn/init.go, we will Initialize each of these authenticaiton package which will return authenticaiton service and corresponding executor. Using that we can then create authnService and executorProvider(I am not sure whether this name) which will keep the map of executors which will be used for the flowexec
# authn/init.go
....
oauthAuthnSvc, oAuthExectuor := authnoauth.Initialize(idpService, userService)
oidcAuthnSvc, oidcExectuor := authnoidc.Initialize(idpService, userService, jwtService)
......
authnService := newAuthenticationService(
...
oauthAuthnSvc,
oidcAuthnSvc,
....
)
...
executorProvider := ...
return authnService, executorProvider
In the authn/oauth/init.go, we can Initialize each of authenticaiton service and corresponding executor as well and return both.
# authn/oauth/init.go
oauthAuthnSvc := newOAuthAuthnService(...)
oauthAuthExceutor := newOAuthAuthExceutor(oauthAuthnSvc)
return oauthAuthnSvc, oauthAuthExceutor
So with this change there won't be need for the executor package.
I like this approach because,
- It keeps the main servicemanager clean, it doesn't need to know about individual authentication service
- It's better if we can keep the authentication service and corresponding executor under the same package
- This will give us add recoveryservice and it's executors in another package and feed into the flowExce.
There was a problem hiding this comment.
Then isn't this introducing a cyclic dependency when we try to construct executor from the internal/authn/oauth package?
There was a problem hiding this comment.
The directory structure I am suggesting will be like this
authn/
oauth2/
service.go
oauthexecutor.go
init.go
github/
service.go
githubexecutor.go
init.go
service.go
handler.go
init.go
There was a problem hiding this comment.
Let's track this as an improvement as discussed offline. This will require refactoring the executors which will conflict with my other pending changes
There was a problem hiding this comment.
Created an issue to track this: #674.
Will push the other suggestion we discussed offline
There was a problem hiding this comment.
Will push the other suggestion we discussed offline
Addressed with https://github.com/asgardeo/thunder/compare/22de2d58dac5aaff9a803b66f09aa5c55ca0718c..86e5674d38cb3863e03a48245ea754a10fd469cc
22de2d5 to
86e5674
Compare
Purpose
$subject