Initiate login flow from oauth before redirecting to gate#624
Initiate login flow from oauth before redirecting to gate#624senthalan merged 2 commits intoasgardeo:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the OAuth authorization flow to use a pre-initialized flow ID instead of passing the application ID. The flow is now initiated before redirecting to the authentication page, providing better separation of concerns between OAuth authorization and flow execution.
Key changes:
- Added
InitiateFlow()method toFlowExecServiceInterfaceto pre-initialize flows without execution - Replaced
applicationIdparameter withflowIdthroughout the authorization flow - Updated integration tests to extract and validate the
flowIdinstead ofapplicationId
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/internal/flow/flowexec/service.go | Added InitiateFlow() method to pre-initialize flows with runtime data |
| backend/internal/flow/common/model/flowmodel.go | Added FlowInitContext struct for flow initialization |
| backend/internal/flow/common/constants/errorconstants.go | Added error constants for flow initialization |
| backend/internal/oauth/oauth2/authz/handler.go | Updated to initiate flow and pass flowId instead of applicationId |
| backend/internal/oauth/oauth2/authz/session_store.go | Added FlowID field to SessionData |
| backend/internal/oauth/oauth2/constants/constants.go | Added FlowID constant |
| backend/internal/oauth/oauth2/authz/init.go | Updated initialization to inject flowExecService |
| backend/internal/oauth/oauth2/granthandlers/init.go | Updated to pass flowExecService to authz initialization |
| backend/internal/oauth/init.go | Updated to accept and pass flowExecService |
| backend/cmd/server/servicemanager.go | Updated to capture and pass flowExecService |
| tests/integration/oauth/authz/utils.go | Updated test helpers to use flowId instead of applicationId |
| tests/integration/oauth/authz/authz_test.go | Updated tests to validate flowId extraction |
| backend/.mockery.public.yml | Added mock configuration for flowexec package |
| backend/tests/mocks/flowexecmock/*.go | Generated mocks for flow execution interfaces |
| backend/internal/oauth/oauth2/authz/handler_test.go | Updated unit tests to inject mock flow exec service |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #624 +/- ##
==========================================
+ Coverage 65.39% 65.49% +0.10%
==========================================
Files 200 200
Lines 17752 17782 +30
Branches 255 250 -5
==========================================
+ Hits 11609 11647 +38
+ Misses 4798 4792 -6
+ Partials 1345 1343 -2
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:
|
836bb44 to
026a00b
Compare
026a00b to
171770a
Compare
| ctx.RuntimeData = make(map[string]string) | ||
| } | ||
| for key, value := range initContext.RuntimeData { | ||
| if _, ok := initContext.RuntimeData[key]; !ok { |
There was a problem hiding this comment.
The condition on line 401 is checking if the key exists in initContext.RuntimeData instead of ctx.RuntimeData. This will always be true since we're iterating over initContext.RuntimeData. The check should be if _, ok := ctx.RuntimeData[key]; !ok to prevent overwriting existing values in the engine context.
| if _, ok := initContext.RuntimeData[key]; !ok { | |
| if _, ok := ctx.RuntimeData[key]; !ok { |
171770a to
34ead24
Compare
34ead24 to
8a35653
Compare
backend/tests/mocks/flowmgtmock/FlowMgtServiceInterface_mock.go
Outdated
Show resolved
Hide resolved
8a35653 to
a730701
Compare
| if ctx.RuntimeData == nil { | ||
| ctx.RuntimeData = make(map[string]string) | ||
| } | ||
| for key, value := range initContext.RuntimeData { | ||
| ctx.RuntimeData[key] = value | ||
| } |
There was a problem hiding this comment.
[nitpick] The runtime data merging logic could be simplified by using a utility function for map merging, especially if this pattern is used elsewhere in the codebase. This would improve consistency and maintainability.
a730701 to
a42d853
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
backend/internal/flow/flowmgt/service.go:45
- The removal of the unexported
init() errormethod from the interface is a breaking change to the interface definition. While this appears intentional, ensure that all implementations of FlowMgtServiceInterface have been updated accordingly and that initialization is now handled differently (e.g., in an Initialize function).
type FlowMgtServiceInterface interface {
RegisterGraph(graphID string, g model.GraphInterface)
GetGraph(graphID string) (model.GraphInterface, bool)
IsValidGraphID(graphID string) bool
}
a42d853 to
266e30c
Compare
266e30c to
27e8de1
Compare
| flowInitCtx := &model.FlowInitContext{ | ||
| ApplicationID: app.AppID, | ||
| FlowType: string(constants.FlowTypeAuthentication), | ||
| RuntimeData: nil, |
There was a problem hiding this comment.
Setting RuntimeData explicitly to nil is unnecessary since the zero value for a map is nil. Consider removing this line or initializing it as an empty map map[string]string{} if you intend to add OAuth-specific runtime data in the future.
| RuntimeData: nil, |
| } | ||
|
|
||
| // ErrorInvalidFlowInitContext defines the error response for invalid flow init context. | ||
| var ErrorInvalidFlowInitContext = serviceerror.ServiceError{ |
There was a problem hiding this comment.
Shall we move this to line 82 where client errors are defined?
| type FlowExecServiceInterface interface { | ||
| Execute(appID, flowID, actionID, flowType string, inputData map[string]string) ( | ||
| *model.FlowStep, *serviceerror.ServiceError) | ||
| InitiateFlow(initContext *model.FlowInitContext) (string, *serviceerror.ServiceError) |
There was a problem hiding this comment.
Having a separate function could be misleading for a outside component which uses this flow execution service right?
In a normal flow execution, we should call Execute() function to both initialize and continue an existing flow. However with InitiateFlow(), someone could confuse which function to call when calling the service. Ideally this should only be called from an internal component.
There was a problem hiding this comment.
As discussed offline, we will be refactoring later
There was a problem hiding this comment.
Lets create an issue and track.. I would like to execute\initialize the flow, without needing the application id.
27e8de1 to
20edcbc
Compare
| // ErrorSerializingRuntimeData defines the error response for errors while serializing runtime data. | ||
| var ErrorSerializingRuntimeData = serviceerror.ServiceError{ | ||
| Code: "FES-5022", | ||
| Type: serviceerror.ServerErrorType, | ||
| Error: "Something went wrong", | ||
| ErrorDescription: "Error serializing runtime data to JSON", | ||
| } |
There was a problem hiding this comment.
The error constant ErrorSerializingRuntimeData is defined but never used in the codebase. The InitiateFlow method doesn't perform any serialization of RuntimeData. Either implement the serialization logic that uses this error or remove the unused constant.
| // ErrorSerializingRuntimeData defines the error response for errors while serializing runtime data. | |
| var ErrorSerializingRuntimeData = serviceerror.ServiceError{ | |
| Code: "FES-5022", | |
| Type: serviceerror.ServerErrorType, | |
| Error: "Something went wrong", | |
| ErrorDescription: "Error serializing runtime data to JSON", | |
| } |
20edcbc to
430785e
Compare
| if len(initContext.RuntimeData) > 0 { | ||
| ctx.RuntimeData = initContext.RuntimeData | ||
| } |
There was a problem hiding this comment.
The condition len(initContext.RuntimeData) > 0 means that if initContext.RuntimeData is nil or empty, ctx.RuntimeData keeps its value from initContext(). Consider explicitly setting ctx.RuntimeData = nil in the else branch or before this check to ensure predictable behavior when RuntimeData is intentionally empty or nil.
| if len(initContext.RuntimeData) > 0 { | |
| ctx.RuntimeData = initContext.RuntimeData | |
| } | |
| ctx.RuntimeData = initContext.RuntimeData |
| } | ||
|
|
||
| // Initiate flow with OAuth context | ||
| logger := log.GetLogger().With(log.String(log.LoggerKeyComponentName, loggerComponentName)) |
There was a problem hiding this comment.
The logger is being created inside handleInitialAuthorizationRequest which is called for every authorization request. Consider initializing the logger once in the handler struct during construction to avoid repeated logger creation overhead.
430785e to
0ed0522
Compare
5851e9f to
1acdcb3
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- frontend/pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
backend/internal/flow/flowexec/service_test.go:1
- Taking the address of a package-level variable creates a pointer to the shared error constant. This should use the error value directly without taking its address:
mockError := flowconstants.ErrorUpdatingContextInStore. Similarly, in handler_test.go line 350, the same issue exists.
/*
1acdcb3 to
8a413cd
Compare
This pull request introduces a new flow initialization mechanism and integrates it into the OAuth authorization process. The changes allow the OAuth authorization handler to pre-initialize a flow with runtime data and store the resulting
flowIdin the session, improving extensibility for future authentication flows. The update also includes dependency injection improvements, new error handling, and test adjustments.Architecture Flow
Depends on asgardeo/javascript#229 for the Gate App to use the new flowId param
Breaking change is for the deployments which hosts the Gate Application hosted outside(Currently, the oauth2/authorize will redirect the Gate with the applicationId, with this change the redirection from the oauth2/authorize will redirect the Gate with the flowId). In those deployments, the hosted applications
asgardeo/reactsdk needs to be updated to0.5.33.