Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request implements SSO functionality and related authentication enhancements along with updates to test configurations and minor code refactoring for better maintainability.
- Introduces SSO support in AuthService, including new methods (loginSSO, clearCurrentUser) and state management.
- Updates various components (SignIn, Pia, Users, Structure, Example, URL) to use the new SSO and authentication flows.
- Adjusts Cypress tests and commands to improve stability and remove unused imports.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/shared/components/header/header.component.html | Added check for the access_type property in rendering technical elements. |
| src/app/services/pia.service.ts | Subscribes to currentUser for conditional loading of example Pia data. |
| src/app/services/auth.service.ts | Enhanced SSO support and refactored logout logic using clearCurrentUser. |
| src/app/modules/users/users.component.ts | Subscribes to currentUser to trigger user loading logic. |
| src/app/modules/structure/structure.component.ts | Injected AuthService and added a subscription; contains an empty ngOnInit. |
| src/app/modules/settings/url/url.component.ts | Updates ssoEnabled flag from introspection response. |
| src/app/modules/pia/pia.component.ts | Uses a subscription to currentUser to conditionally load Pia data. |
| src/app/modules/pia/example/example.component.ts | Injected AuthService and updated subscription logic. |
| src/app/modules/home/home.component.ts | Removed an unused lodash import. |
| src/app/modules/home/forms/sign-in/sign-in.component.ts | Added SSO login function and updated dependency injection for AuthService. |
| src/app/modules/home/forms/sign-in/sign-in.component.html | Added a link to trigger SSO login when enabled. |
| src/app/modules/entries/forms/new-pia/new-pia.component.ts | Updated subscription to currentUser for structure list loading. |
| src/app/modules/base/base.component.ts | Added import for AuthService to support SSO-related functionality. |
| cypress/support/commands.js | Introduced an explicit wait in navigation command. |
| cypress/e2e/pia-angular/entries/entries_table.spec.js | Adjusted forced click usage for more robust test interactions. |
| cypress/e2e/pia-angular/dpo/dpo_and_pia_validation.spec.js | Minor update; removal of an unused variable from test logic. |
| cypress.config.ts | Disabled test isolation to allow shared state between tests. |
| if (!examples) { | ||
| await this.importData(piaExample, 'EXAMPLE', false, true); | ||
| this.authService.currentUser.subscribe({ | ||
| complete: () => { |
There was a problem hiding this comment.
Using the 'complete' callback on the subscription to currentUser may not trigger if the observable never completes; consider using the 'next' callback or an operator like 'first' to ensure the initialization logic runs.
| complete: () => { | |
| next: () => { |
| ) { | ||
| this.loadUsers(); | ||
| this.authService.currentUser.subscribe({ | ||
| complete: () => { |
There was a problem hiding this comment.
The use of the 'complete' callback here might not execute if the observable remains open; consider switching to the 'next' callback to ensure loadUsers() is called as expected.
| complete: () => { | |
| next: () => { |
| private authService: AuthService | ||
| ) { | ||
| this.authService.currentUser.subscribe({ | ||
| complete: () => { |
There was a problem hiding this comment.
Relying on the 'complete' callback for executing initialization logic may not work as intended with a BehaviorSubject; consider using the 'next' callback or appropriate operators to ensure the code is executed.
| complete: () => { | |
| next: () => { |
| } | ||
|
|
||
| this.authService.currentUser.subscribe({ | ||
| complete: () => { |
There was a problem hiding this comment.
Using 'complete' in the subscription may never invoke the contained logic; using the 'next' callback or 'first' operator would provide more reliable initialization of Pia data.
| complete: () => { | |
| next: () => { |
| public piaService: PiaService | ||
| ) { | ||
| this.authService.currentUser.subscribe({ | ||
| complete: () => { |
There was a problem hiding this comment.
The subscription uses a 'complete' callback which might not be triggered; consider using the 'next' callback to ensure that the Pia retrieval code executes as expected.
| complete: () => { | |
| next: () => { |
| console.error(err); | ||
| }); | ||
| this.authService.currentUser.subscribe({ | ||
| complete: () => { |
There was a problem hiding this comment.
Relying on the 'complete' callback could lead to missed initialization if the observable stays active; switching to the 'next' callback would ensure that the structure list is loaded reliably.
| complete: () => { | |
| next: () => { |
| this.subscription = this.measureService.behaviorSubject.subscribe( | ||
| val => { | ||
| this.measureToRemoveFromTags = val; | ||
| } | ||
| ); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| async ngOnInit(): Promise<void> {} | ||
|
|
There was a problem hiding this comment.
[nitpick] An empty ngOnInit method may indicate that initialization logic is split between the constructor and lifecycle hooks; consider consolidating the logic in ngOnInit for clarity and standard Angular practice.
| this.subscription = this.measureService.behaviorSubject.subscribe( | |
| val => { | |
| this.measureToRemoveFromTags = val; | |
| } | |
| ); | |
| } | |
| }); | |
| } | |
| async ngOnInit(): Promise<void> {} | |
| this.subscription = this.measureService.behaviorSubject.subscribe( | |
| val => { | |
| this.measureToRemoveFromTags = val; | |
| } | |
| ); | |
| } | |
| }); | |
| } |
brunto
force-pushed
the
sso
branch
2 times, most recently
from
5532130 to
10cca94
Compare
# Conflicts: # src/app/modules/pia/example/example.component.ts # src/app/services/auth.service.ts # src/app/shared/components/header/header.component.html
2 participants
This pull request introduces several changes across the codebase, primarily focusing on enhancing Single Sign-On (SSO) functionality, improving Cypress test configurations, and adding minor fixes and enhancements. Below is a categorized summary of the most significant changes:
SSO Implementation and Authentication Enhancements:
AuthService, including methods to handle SSO login (loginSSO) and logout, and a newssoEnabledproperty to track SSO availability. TheclearCurrentUsermethod was introduced to centralize user cleanup logic. [1] [2] [3] [4]SignInComponentto display an SSO login option when SSO is enabled and to handle SSO login via a newloginSSOmethod. [1] [2] [3]PiaComponent,StructureComponent,ExampleComponent, etc.) to subscribe toAuthService.currentUserfor initializing data after authentication. [1] [2] [3]Cypress Test Improvements:
cypress.config.tsto allow shared state between tests.Codebase Enhancements and Refactoring:
logoutcalls with the newclearCurrentUsermethod inAuthServicefor consistency.home.component.ts, to clean up the codebase.Minor Fixes and UI Updates:
access_typewhen rendering technical access elements.AuthServicein multiple components to support the new SSO-related functionality. [1] [2] [3]These changes improve the authentication flow, enhance test stability, and clean up the codebase for better maintainability.