[NAE-2118] Implement OpenID Connector Auth for Admin node #286
Conversation
- create login-sso.component - add login sso button in login-form.component - update schema.ts with sso configuration
- implement redirect to iam
- implement backend call with access token
- add realm id request placeholder
- preconfigure nae.json for local development
WalkthroughThis update introduces Single Sign-On (SSO) functionality into the authentication flow. It adds SSO configuration options, updates interfaces, and implements new Angular components for SSO login. Localization files are extended with SSO button labels, and the login form is modified to conditionally display the SSO button based on configuration. Supporting unit tests and module declarations are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginForm
participant SSOButton
participant SSOProvider
participant AppBackend
User->>LoginForm: Loads login page
LoginForm->>SSOButton: Conditionally renders if SSO enabled
User->>SSOButton: Clicks SSO login button
SSOButton->>SSOProvider: Redirects to SSO provider's auth URL
SSOProvider-->>User: Authenticates and redirects back with code
User->>LoginForm: Returns with authorization code in URL
LoginForm->>AppBackend: Exchanges code for access token (via refreshUrl)
AppBackend-->>LoginForm: Returns access token
LoginForm->>User: Navigates to post-login route
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
projects/netgrif-components-core/src/lib/forms/login/login-sso/abstract-login-sso.component.spec.ts (1)
1-2: Placeholder spec without tests
The spec file currently contains only a TODO comment and no substantive tests for theAbstractLoginSsoComponent. Unit tests should cover redirect initiation, authorization code handling, token exchange, and error scenarios.Would you like help scaffolding a baseline test suite for this component?
projects/netgrif-components/src/lib/forms/login/login-form.component.html (1)
43-45: SSO button rendering block
The<nc-login-sso>component is conditionally rendered viashowSsoButton. For consistent UX, consider adding spacing (e.g.,fxLayoutGap="8px") or margin classes to align it with existing buttons, and verify that the embedded button has an appropriatearia-labelfor accessibility.projects/netgrif-components/src/lib/forms/login/login-form.module.ts (1)
7-7: Unify quotation styleEvery other import in this file (and most of the repo) uses single quotes. Sticking to a single quoting convention avoids noisy diffs and keeps the tslint/prettier rules happy.
-import {LoginSsoComponent} from "./login-sso/login-sso.component"; +import {LoginSsoComponent} from './login-sso/login-sso.component';projects/netgrif-components/src/lib/forms/login/login-sso/login-sso.component.html (1)
1-9: Add explicit aria-label to improve accessibility & E2E testabilityScreen-reader users and automated test suites don’t have a stable, deterministic identifier for this button.
Consider adding anaria-label(ormatTooltip) that matches the visible translation key.-<button color="primary" (click)="redirectToSso()" [disabled]="(loading | async)" mat-raised-button fxLayout="row" +<button color="primary" + aria-label="{{ 'forms.login.ssoButton' | translate }}" + (click)="redirectToSso()" + [disabled]="(loading | async)" + mat-raised-button fxLayout="row"projects/netgrif-components/src/lib/forms/login/login-sso/login-sso.component.ts (2)
7-11: Enable OnPush change detection for a purely presentational componentThe component has no mutable internal state – it only proxies observables from the abstract superclass.
Switching toChangeDetectionStrategy.OnPushavoids unnecessary change-detection cycles and is consistent with Angular best practices for performance-sensitive pages.@Component({ selector: 'nc-login-sso', templateUrl: './login-sso.component.html', styleUrls: ['./login-sso.component.scss'], + changeDetection: ChangeDetectionStrategy.OnPush })Remember to add
ChangeDetectionStrategyto the import list.
13-21: Minor naming & visibility nits in constructor parameters
_activeRouteris a typo – Angular convention isActivatedRoute, often shortened to_activatedRoute.- None of the injected services are used directly in this concrete class; they are only forwarded to
super. Marking themprivate readonlyremoves the instance fields and clarifies intent.- protected _activeRouter: ActivatedRoute, + private readonly _activatedRoute: ActivatedRoute,…and change the
supercall accordingly.
Purely cosmetic, but keeps the code-base consistent.projects/netgrif-components/src/lib/forms/login/login-form.component.ts (1)
11-13:_configproperty seems unused inside this class
ConfigurationServiceis forwarded tosuperbut never referenced afterwards.
If no additional logic is planned, mark the parameterprivateto avoid an extra instance field:- constructor(formBuilder: FormBuilder, protected _userService: UserService, protected _config: ConfigurationService) { + constructor(formBuilder: FormBuilder, protected _userService: UserService, private _config: ConfigurationService) {projects/netgrif-components-core/src/lib/forms/login/abstract-login-form.component.ts (1)
8-10: Inconsistent import quote styleMost imports in this file use single quotes; these lines use double quotes. Aligning the style prevents noisy diffs.
-import {ConfigurationService} from "../../configuration/configuration.service"; -import {Sso} from "../../../commons/schema"; +import {ConfigurationService} from '../../configuration/configuration.service'; +import {Sso} from '../../../commons/schema';projects/netgrif-components-core/src/lib/forms/login/login-sso/abstract-login-sso.component.ts (2)
35-35: Redundant double negation
if (params.code)is sufficient;!!params.codeadds no value.
Same applies to!!tokenon line 67.
95-101: Error message misleads users
Displayingforms.login.wrongCredentialswhen an SSO token exchange fails conflates two different error conditions. Consider adding a dedicated translation key, e.g.forms.login.ssoError.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
nae.json(2 hunks)projects/netgrif-components-core/src/assets/i18n/de.json(1 hunks)projects/netgrif-components-core/src/assets/i18n/en.json(1 hunks)projects/netgrif-components-core/src/assets/i18n/sk.json(1 hunks)projects/netgrif-components-core/src/commons/schema.ts(1 hunks)projects/netgrif-components-core/src/lib/forms/login/abstract-login-form.component.ts(4 hunks)projects/netgrif-components-core/src/lib/forms/login/login-sso/abstract-login-sso.component.spec.ts(1 hunks)projects/netgrif-components-core/src/lib/forms/login/login-sso/abstract-login-sso.component.ts(1 hunks)projects/netgrif-components-core/src/lib/forms/public-api.ts(1 hunks)projects/netgrif-components/src/lib/forms/login/login-form.component.html(1 hunks)projects/netgrif-components/src/lib/forms/login/login-form.component.ts(1 hunks)projects/netgrif-components/src/lib/forms/login/login-form.module.ts(1 hunks)projects/netgrif-components/src/lib/forms/login/login-sso/login-sso.component.html(1 hunks)projects/netgrif-components/src/lib/forms/login/login-sso/login-sso.component.spec.ts(1 hunks)projects/netgrif-components/src/lib/forms/login/login-sso/login-sso.component.ts(1 hunks)projects/netgrif-components/src/lib/forms/public-api.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
projects/netgrif-components/src/lib/forms/login/login-sso/login-sso.component.ts (1)
projects/netgrif-components/src/lib/forms/login/login-form.component.ts (1)
Component(5-14)
projects/netgrif-components/src/lib/forms/login/login-form.component.ts (1)
projects/netgrif-components/src/lib/forms/login/login-sso/login-sso.component.ts (1)
Component(7-24)
projects/netgrif-components-core/src/lib/forms/login/abstract-login-form.component.ts (1)
projects/netgrif-components-core/src/commons/schema.ts (1)
Sso(54-60)
🪛 HTMLHint (1.5.0)
projects/netgrif-components/src/lib/forms/login/login-sso/login-sso.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🪛 Biome (1.9.4)
projects/netgrif-components-core/src/lib/forms/login/login-sso/abstract-login-sso.component.ts
[error] 35-35: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 67-67: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (20)
- GitHub Check: Matrix Test (18)
- GitHub Check: Test with SonarCloud
🔇 Additional comments (8)
projects/netgrif-components-core/src/assets/i18n/en.json (1)
314-314: Translation key added correctly
ThessoButtonentry underforms.loginis well-formed and reads"Log with SSO". No syntax issues detected.projects/netgrif-components-core/src/assets/i18n/de.json (1)
314-314: German SSO translation added
The new"ssoButton": "SSO-Anmeldung"entry underforms.loginis correctly inserted and valid JSON.projects/netgrif-components-core/src/assets/i18n/sk.json (1)
314-314: Slovak SSO translation added
The"ssoButton": "Prihlásiť sa pomocou SSO"key is properly placed in theforms.loginsection with valid JSON syntax.projects/netgrif-components/src/lib/forms/login/login-form.module.ts (1)
11-12: Declaration/export looks good
LoginSsoComponentis properly declared and exported, so consumers of the module will see it without extra plumbing.
No issues spotted.projects/netgrif-components/src/lib/forms/public-api.ts (1)
4-4: Public API updated correctlyThe new component is exposed to library consumers – exactly what we need. 👍
projects/netgrif-components-core/src/lib/forms/public-api.ts (1)
3-3: Abstract SSO component export addedMakes the base class reusable for external extensions. Looks good.
projects/netgrif-components-core/src/commons/schema.ts (1)
49-60: Schema extension LGTM – but check naming consistencyThe interface covers the minimum needed fields. Make sure the runtime config uses the same property names (
redirectUrl,refreshUrl, …) – mismatches will surface only at runtime because this is a.d.tsfile.No blocking issues.
projects/netgrif-components-core/src/lib/forms/login/login-sso/abstract-login-sso.component.ts (1)
75-82: Possible NPE & unsafejoinonscopes
Ifscopesisundefined(mis-configured environment)joinwill crash. Either default to an empty array or throw a clear error earlier.- scope: this._ssoConfig.scopes.join(' '), + scope: (this._ssoConfig.scopes ?? []).join(' '),
| // todo 2118 | ||
|
|
||
| xdescribe('LoginSsoComponent', () => { | ||
| let component: LoginSsoComponent; | ||
| let fixture: ComponentFixture<LoginSsoComponent>; | ||
|
|
||
| beforeEach(async () => { | ||
| await TestBed.configureTestingModule({ | ||
| declarations: [ LoginSsoComponent ] | ||
| }) | ||
| .compileComponents(); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| fixture = TestBed.createComponent(LoginSsoComponent); | ||
| component = fixture.componentInstance; | ||
| fixture.detectChanges(); | ||
| }); | ||
|
|
||
| it('should create', () => { | ||
| expect(component).toBeTruthy(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Disabled test suite hides compilation/runtime issues
The whole spec is wrapped in xdescribe, so it never runs.
- Potential template-compilation errors inside
LoginSsoComponentwill stay undetected. - Test coverage tools will ignore the component entirely.
Unless there is a strong, short-term reason to skip, turn it into an active suite (or delete it until ready).
-xdescribe('LoginSsoComponent', () => {
+describe('LoginSsoComponent', () => {If the component depends on external modules (Material, TranslateLib, etc.) add them to the testing module to prevent runtime failures.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // todo 2118 | |
| xdescribe('LoginSsoComponent', () => { | |
| let component: LoginSsoComponent; | |
| let fixture: ComponentFixture<LoginSsoComponent>; | |
| beforeEach(async () => { | |
| await TestBed.configureTestingModule({ | |
| declarations: [ LoginSsoComponent ] | |
| }) | |
| .compileComponents(); | |
| }); | |
| beforeEach(() => { | |
| fixture = TestBed.createComponent(LoginSsoComponent); | |
| component = fixture.componentInstance; | |
| fixture.detectChanges(); | |
| }); | |
| it('should create', () => { | |
| expect(component).toBeTruthy(); | |
| }); | |
| }); | |
| // todo 2118 | |
| describe('LoginSsoComponent', () => { | |
| let component: LoginSsoComponent; | |
| let fixture: ComponentFixture<LoginSsoComponent>; | |
| beforeEach(async () => { | |
| await TestBed.configureTestingModule({ | |
| declarations: [ LoginSsoComponent ] | |
| }) | |
| .compileComponents(); | |
| }); | |
| beforeEach(() => { | |
| fixture = TestBed.createComponent(LoginSsoComponent); | |
| component = fixture.componentInstance; | |
| fixture.detectChanges(); | |
| }); | |
| it('should create', () => { | |
| expect(component).toBeTruthy(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In
projects/netgrif-components/src/lib/forms/login/login-sso/login-sso.component.spec.ts
between lines 5 and 27, the test suite is disabled using xdescribe, which
prevents the tests from running and hides potential compilation or runtime
errors. To fix this, remove the 'x' prefix to activate the describe block so the
tests execute. Additionally, ensure that any external modules required by
LoginSsoComponent, such as Material or TranslateLib, are imported and included
in the TestBed configuration to avoid runtime failures during testing.
| "sso": { | ||
| "enable": true, | ||
| "clientId": "dev-cluster-worker", | ||
| "redirectUrl": "https://iam.mudu.dev.netgrif.cloud/realms/netgrif-cloud-testing/protocol/openid-connect/auth", | ||
| "refreshUrl": "http://localhost:8800/api/auth/login", | ||
| "scopes": ["openid","email","profile","roles"] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Mixed-content risk: refreshUrl uses HTTP while the app is likely served over HTTPS
Browsers will block XMLHttpRequests from an HTTPS origin to an HTTP endpoint, breaking the token refresh flow in production.
Please verify that the service is reachable via HTTPS or proxy the call through the same origin.
Mixed-content risk: refreshUrl uses HTTP while the app is likely served over HTTPS
Browsers will block XMLHttpRequests from an HTTPS origin to an HTTP endpoint, breaking the token refresh flow in production.
Please verify that the service is reachable via HTTPS or proxy the call through the same origin.
🤖 Prompt for AI Agents
In nae.json around lines 23 to 29, the refreshUrl is set to use HTTP which can
cause mixed-content issues when the app is served over HTTPS. Update the
refreshUrl to use HTTPS if the service supports it, or configure a proxy on the
same HTTPS origin to forward requests to the HTTP endpoint, ensuring secure and
unblocked token refresh requests.
| let ssoConfig: Sso = this._config.getConfigurationSubtree(['providers', 'auth', 'sso']) | ||
| this.showSsoButton = ssoConfig?.enable | ||
| } |
There was a problem hiding this comment.
Defensive defaults & const instead of let
showSsoButton is declared boolean but can currently be undefined.
Also, ssoConfig isn’t reassigned – mark it const.
- let ssoConfig: Sso = this._config.getConfigurationSubtree(['providers', 'auth', 'sso'])
- this.showSsoButton = ssoConfig?.enable
+ const ssoConfig: Sso | undefined =
+ this._config.getConfigurationSubtree(['providers', 'auth', 'sso']);
+ this.showSsoButton = !!ssoConfig?.enable;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let ssoConfig: Sso = this._config.getConfigurationSubtree(['providers', 'auth', 'sso']) | |
| this.showSsoButton = ssoConfig?.enable | |
| } | |
| const ssoConfig: Sso | undefined = | |
| this._config.getConfigurationSubtree(['providers', 'auth', 'sso']); | |
| this.showSsoButton = !!ssoConfig?.enable; | |
| } |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/forms/login/abstract-login-form.component.ts
around lines 39 to 41, change the declaration of ssoConfig from let to const
since it is not reassigned. Also, ensure showSsoButton is assigned a boolean
value by providing a defensive default (e.g., false) when ssoConfig or
ssoConfig.enable is undefined, so showSsoButton never becomes undefined.
| private _ssoConfig: Sso; | ||
| protected loading: LoadingEmitter; | ||
|
|
||
| protected constructor( | ||
| protected _config: ConfigurationService, | ||
| protected _http: HttpClient, | ||
| protected _snackbar: SnackBarService, | ||
| protected _log: LoggerService, | ||
| protected _router: Router, | ||
| protected _activeRouter: ActivatedRoute, | ||
| protected _translate: TranslateService | ||
| ) { | ||
| this._ssoConfig = this._config.getConfigurationSubtree(['providers', 'auth', 'sso']); | ||
| this.loading = new LoadingEmitter(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing SSO configuration
_config.getConfigurationSubtree() can legally return undefined if the subtree is not present. Directly dereferencing the resulting object (this._ssoConfig.clientId, this._ssoConfig.scopes, …) will then throw at run time.
- this._ssoConfig = this._config.getConfigurationSubtree(['providers', 'auth', 'sso']);
+ const subtree = this._config.getConfigurationSubtree(['providers', 'auth', 'sso']);
+ if (!subtree) {
+ this._log.error('SSO configuration subtree is missing – SSO button will not work');
+ return;
+ }
+ this._ssoConfig = subtree;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/forms/login/login-sso/abstract-login-sso.component.ts
around lines 20 to 33, the assignment of this._ssoConfig directly from
_config.getConfigurationSubtree() can result in undefined if the configuration
subtree is missing. To fix this, add a guard to check if the returned value is
undefined and handle that case appropriately, such as initializing
this._ssoConfig with a default object or adding null checks before accessing its
properties to prevent runtime errors.
| this._log.debug('Handling access token: ' + params.code) | ||
| const token$ = this.getToken({ |
There was a problem hiding this comment.
Authorization code is sensitive – do not log it
this._log.debug('Handling access token: ' + params.code) exposes the OAuth 2.0 authorization-code in logs, creating an unnecessary attack surface. Remove the value or mask it.
- this._log.debug('Handling access token: ' + params.code)
+ this._log.debug('Handling authorization-code callback')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this._log.debug('Handling access token: ' + params.code) | |
| const token$ = this.getToken({ | |
| this._log.debug('Handling authorization-code callback') | |
| const token$ = this.getToken({ |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/forms/login/login-sso/abstract-login-sso.component.ts
at lines 57-58, the code logs the OAuth 2.0 authorization code in plain text,
which is sensitive information. Modify the debug log to either remove the
authorization code entirely or replace it with a masked or generic placeholder
to avoid exposing sensitive data in logs.
| this._activeRouter.queryParams.subscribe((params) => { | ||
| if (!!params.code) { | ||
| this.loginFromCode(params); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Subscription is never disposed – memory-leak risk
The component subscribes to queryParams in the constructor but does not retain nor unsubscribe from the resulting Subscription. When the component is destroyed, the stream will keep the component instance alive.
+import {Subscription} from 'rxjs';
…
- this._activeRouter.queryParams.subscribe((params) => {
+ this._paramsSub = this._activeRouter.queryParams.subscribe(params => {
if (params.code) {
this.loginFromCode(params);
}
});and in the class body / ngOnDestroy:
- ngOnDestroy(): void {
- this.loading.complete();
- }
+ private _paramsSub?: Subscription;
+
+ ngOnDestroy(): void {
+ this._paramsSub?.unsubscribe();
+ this.loading.complete();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this._activeRouter.queryParams.subscribe((params) => { | |
| if (!!params.code) { | |
| this.loginFromCode(params); | |
| } | |
| }); | |
| } | |
| // at the top of the file | |
| import { Subscription } from 'rxjs'; | |
| … | |
| export class AbstractLoginSsoComponent implements OnDestroy { | |
| // add a field to hold the subscription | |
| private _paramsSub?: Subscription; | |
| constructor( | |
| private _activeRouter: ActivatedRoute, | |
| // … | |
| ) { | |
| // capture the subscription so we can unsubscribe later | |
| this._paramsSub = this._activeRouter.queryParams.subscribe(params => { | |
| if (params.code) { | |
| this.loginFromCode(params); | |
| } | |
| }); | |
| } | |
| // … | |
| ngOnDestroy(): void { | |
| // tear down the router subscription | |
| this._paramsSub?.unsubscribe(); | |
| this.loading.complete(); | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/forms/login/login-sso/abstract-login-sso.component.ts
around lines 34 to 39, the subscription to this._activeRouter.queryParams is
created but never stored or unsubscribed, causing a potential memory leak. Fix
this by storing the subscription in a class property and then unsubscribing from
it in the ngOnDestroy lifecycle method to properly clean up when the component
is destroyed.
Description
Implemented SSO login button, that is configurable. Button redirects to configured OIDC provider login form and then sends received
authorization_codeto gateway for further processingImplements NAE-2118
Dependencies
No new dependencies were introduced
Third party dependencies
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
Manually
Test Configuration
Checklist:
Summary by CodeRabbit