GDB-14665: Fix regression to allow multiple login strategies#2995
Open
yordanalexandrov wants to merge 1 commit into
Open
GDB-14665: Fix regression to allow multiple login strategies#2995yordanalexandrov wants to merge 1 commit into
yordanalexandrov wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the authentication flow to support multiple login strategies (GraphDB token vs OpenID) by introducing explicit strategy selection in the API layer and updating the Workbench login UI and tests to match the new AuthenticationService.login(...) signature.
Changes:
- Updated login UI to display both GraphDB and OpenID login options and route login through a single handler.
- Modified
AuthenticationService.loginto accept a singleloginDataobject (optional) and set the appropriate auth strategy. - Refined auth strategy resolution based on authenticated user state and token prefix, and updated unit/integration tests accordingly.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/workbench/src/app/pages/login/login-page.component.ts | Updates login handler to support OpenID vs username/password via a single method and new service signature. |
| packages/workbench/src/app/pages/login/login-page.component.html | Renders multiple login strategy buttons within one form. |
| packages/workbench/src/app/pages/login/login-page.component.scss | Adjusts layout for multiple login actions. |
| packages/shared-components/src/components/onto-test-context/onto-test-context.tsx | Updates helper to call AuthenticationService.login({ username, password }). |
| packages/api/src/services/tracking/test/tracking.service.spec.ts | Updates tests to match the new login signature. |
| packages/api/src/services/domain/security/test/security.service.spec.ts | Updates tests to match the new login signature. |
| packages/api/src/services/domain/security/test/authentication.service.spec.ts | Updates tests for new login signature (but still has an async assertion issue). |
| packages/api/src/services/domain/security/authentication.service.ts | Changes login to accept optional LoginData and select/set strategy accordingly. |
| packages/api/src/services/domain/security/authentication-storage.service.ts | Splits token-type detection into isGDBToken vs isOpenIDToken. |
| packages/api/src/services/domain/security/auth-strategy-resolver.ts | Adds setStrategy and enhances resolution logic based on user/token state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yordanalexandrov
force-pushed
the
gdb-14665-fix-regression-allow-multiple-login-strategies
branch
from
6ca1f21 to
6e4646b
Compare
## What Refactored authentication logic to support multiple login strategies based on user tokens. ## Why This change addresses a regression that prevented the application from correctly handling multiple authentication strategies, improving user experience and flexibility in login options. ## How - Introduced `setStrategy` method to manually set authentication strategies. - Updated `resolveStrategy` to return the appropriate strategy based on user authentication state. - Modified login component to show correct multiple login strategies. - Modified `login` method of `AuthenticationService` to set appropriate strategy based on login details. - Adjusted tests to reflect changes in the login method signature. - As the login manually forces a strategy, the `BaseGdbLoginStrategy` is no longer needed and the login method can go th the `GdbTokenAuthStrategy` directly. No need to update strategy after login either.
yordanalexandrov
force-pushed
the
gdb-14665-fix-regression-allow-multiple-login-strategies
branch
from
6e4646b to
c70b894
Compare
|
|
|
||
| private assertIsLoginData(value: unknown): asserts value is LoginData { | ||
| if (typeof value !== 'object' || value === null || !('username' in value) || !('password' in value)) { | ||
| throw new Error('Invalid login data. Expected an object with username and password properties.'); |
Collaborator
There was a problem hiding this comment.
The above is minor, but in general I wonder do we really need this runtime validation here?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Refactored authentication logic to support multiple login strategies based on user tokens.
Why
This change addresses a regression that prevented the application from correctly handling multiple authentication strategies, improving user experience and flexibility in login options.
How
setStrategymethod to manually set authentication strategies.resolveStrategyto return the appropriate strategy based on user authentication state.loginmethod ofAuthenticationServiceto set appropriate strategy based on login details.Testing
Screenshots
Checklist