Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NAS-133624 / 25.10 / Support for single time user passwords in STIG mode #11427

Merged
merged 13 commits into from
Jan 30, 2025
1 change: 1 addition & 0 deletions src/app/enums/account-attribute.enum.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export enum AccountAttribute {
Local = 'LOCAL',
SysAdmin = 'SYS_ADMIN',
Otpw = 'OTPW',
}
4 changes: 4 additions & 0 deletions src/app/helptext/account/user-form.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,15 @@ export const helptextUsers = {
<i>No:</i> Requires adding a <b>Password</b> to the account. The \
account can use the saved <b>Password</b> to authenticate with \
password-based services.'),
user_form_auth_one_time_pw_tooltip: T('Temporary password will be generated and shown to you once form is saved. \
<br><br>This password is only valid for one login within 24 hours and does not persist across reboots. \
<br><br>User will be encouraged to choose their own password after they login for the first time.'),
user_form_shell_tooltip: T('Select the shell to use for local and SSH logins.'),
user_form_lockuser_tooltip: T('Prevent the user from logging in or \
using password-based services until this option is unset. Locking an \
account is only possible when <b>Disable Password</b> is <i>No</i> and \
a <b>Password</b> has been created for the account.'),
user_form_smb_tooltip: T('Set to allow user to authenticate to Samba shares.'),
smbBuiltin: T('Cannot be enabled for built-in users.'),
smbStig: T('Local user accounts using NTLM authentication are not permitted when TrueNAS is running in an enhanced security mode.'),
};
1 change: 1 addition & 0 deletions src/app/interfaces/api/api-call-directory.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ export interface ApiCallDirectory {

// Auth
'auth.generate_token': { params: void; response: string };
'auth.generate_onetime_password': { params: [{ username: string }]; response: string };
'auth.login_ex': { params: [LoginExQuery]; response: LoginExResponse };
'auth.login_ex_continue': { params: [LoginExOtpTokenQuery]; response: LoginExResponse };
'auth.logout': { params: void; response: void };
Expand Down
6 changes: 6 additions & 0 deletions src/app/interfaces/user.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export interface UserUpdate {
full_name?: string;
email?: string;
password?: string;
random_password?: boolean | null;
password_disabled?: boolean;
locked?: boolean;
smb?: boolean;
Expand All @@ -71,3 +72,8 @@ export interface SetPasswordParams {
old_password: string;
new_password: string;
}

export enum UserStigPasswordOption {
AlexKarpov98 marked this conversation as resolved.
Show resolved Hide resolved
DisablePassword = 'DISABLE_PASSWORD',
OneTimePassword = 'ONE_TIME_PASSWORD',
}
8 changes: 8 additions & 0 deletions src/app/modules/auth/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import { adminUiInitialized } from 'app/store/admin-panel/admin.actions';
export class AuthService {
@LocalStorage() private token: string | undefined | null;
protected loggedInUser$ = new BehaviorSubject<LoggedInUser | null>(null);
isOptwPasswordChanged$ = new BehaviorSubject<boolean>(false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kinda weird to have this here so that two other pieces of code can talk to each other.
See if it's possible to inject TwoFactorGuardService in FirstLoginDialogComponent directly and have that property there directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@undsoft I tried to move it to the TwoFactorGuardService but faced issues.
The main issue is that we should reset that value if user logs out, because if it's a new login with OTPW - the full screen dialog will not show up since password is considered as already changed.


/**
* This is 10 seconds less than 300 seconds which is the default life
Expand Down Expand Up @@ -155,6 +156,13 @@ export class AuthService {
);
}

isOtpwUser(): Observable<boolean> {
AlexKarpov98 marked this conversation as resolved.
Show resolved Hide resolved
return this.user$.pipe(
filter(Boolean),
map((user) => user.account_attributes.includes(AccountAttribute.Otpw)),
);
}

/**
* Checks whether user has any of the supplied roles.
* Does not ensure that user was loaded.
Expand Down
32 changes: 32 additions & 0 deletions src/app/modules/auth/two-factor-guard.service.spec.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { MatDialog } from '@angular/material/dialog';
import { ActivatedRouteSnapshot, Router, RouterStateSnapshot } from '@angular/router';
import { SpectatorService, createServiceFactory, mockProvider } from '@ngneat/spectator/jest';
import { BehaviorSubject, firstValueFrom, of } from 'rxjs';
import { GlobalTwoFactorConfig, UserTwoFactorConfig } from 'app/interfaces/two-factor-config.interface';
import { AuthService } from 'app/modules/auth/auth.service';
import { TwoFactorGuardService } from 'app/modules/auth/two-factor-guard.service';
import { DialogService } from 'app/modules/dialog/dialog.service';
import { StigFirstLoginDialogComponent } from 'app/pages/credentials/users/stig-first-login-dialog/stig-first-login-dialog.component';
import { WebSocketStatusService } from 'app/services/websocket-status.service';

describe('TwoFactorGuardService', () => {
Expand All @@ -14,6 +16,8 @@ describe('TwoFactorGuardService', () => {
const userTwoFactorConfig$ = new BehaviorSubject<UserTwoFactorConfig | null>(null);
const getGlobalTwoFactorConfig = jest.fn(() => of(null as GlobalTwoFactorConfig | null));
const hasRole$ = new BehaviorSubject(false);
const isOtpwUser$ = new BehaviorSubject(false);
const isOptwPasswordChanged$ = new BehaviorSubject(false);

const createService = createServiceFactory({
service: TwoFactorGuardService,
Expand All @@ -26,6 +30,13 @@ describe('TwoFactorGuardService', () => {
userTwoFactorConfig$,
getGlobalTwoFactorConfig,
hasRole: jest.fn(() => hasRole$),
isOtpwUser: jest.fn(() => isOtpwUser$),
isOptwPasswordChanged$,
}),
mockProvider(MatDialog, {
open: jest.fn(() => ({
afterClosed: () => of(true),
})),
}),
mockProvider(DialogService, {
fullScreenDialog: jest.fn(() => of(undefined)),
Expand Down Expand Up @@ -98,4 +109,25 @@ describe('TwoFactorGuardService', () => {
expect(spectator.inject(DialogService).fullScreenDialog).toHaveBeenCalled();
expect(spectator.inject(Router).navigate).toHaveBeenCalledWith(['/two-factor-auth']);
});

it('handles STIG first login for user to proceed with changing one-time password and setting up 2FA', async () => {
isAuthenticated$.next(true);
getGlobalTwoFactorConfig.mockReturnValue(of({ enabled: true } as GlobalTwoFactorConfig));
userTwoFactorConfig$.next({ secret_configured: false } as UserTwoFactorConfig);
isOtpwUser$.next(true);

const isAllowed = await firstValueFrom(
spectator.service.canActivateChild({} as ActivatedRouteSnapshot, { url: '/dashboard' } as RouterStateSnapshot),
);
expect(isAllowed).toBe(true);

expect(spectator.inject(MatDialog).open).toHaveBeenCalledWith(StigFirstLoginDialogComponent, {
maxWidth: '100vw',
maxHeight: '100vh',
height: '100%',
width: '100%',
panelClass: 'full-screen-modal',
disableClose: true,
});
});
});
27 changes: 26 additions & 1 deletion src/app/modules/auth/two-factor-guard.service.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Injectable } from '@angular/core';
import { MatDialog } from '@angular/material/dialog';
import {
ActivatedRouteSnapshot, RouterStateSnapshot, Router, CanActivateChild,
} from '@angular/router';
Expand All @@ -10,6 +11,7 @@ import {
import { Role } from 'app/enums/role.enum';
import { AuthService } from 'app/modules/auth/auth.service';
import { DialogService } from 'app/modules/dialog/dialog.service';
import { StigFirstLoginDialogComponent } from 'app/pages/credentials/users/stig-first-login-dialog/stig-first-login-dialog.component';
import { WebSocketStatusService } from 'app/services/websocket-status.service';

@UntilDestroy()
Expand All @@ -23,6 +25,7 @@ export class TwoFactorGuardService implements CanActivateChild {
private wsStatus: WebSocketStatusService,
private dialogService: DialogService,
private translate: TranslateService,
private matDialog: MatDialog,
) { }

canActivateChild(_: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<boolean> {
Expand All @@ -42,8 +45,17 @@ export class TwoFactorGuardService implements CanActivateChild {
this.authService.userTwoFactorConfig$.pipe(take(1)),
this.authService.getGlobalTwoFactorConfig(),
this.authService.hasRole([Role.FullAdmin]).pipe(take(1)),
this.authService.isOtpwUser().pipe(take(1)),
this.authService.isOptwPasswordChanged$.asObservable().pipe(take(1)),
]).pipe(
switchMap(([userConfig, globalConfig, isFullAdmin]) => {
switchMap(([userConfig, globalConfig, isFullAdmin, isOtpwUser, isOptwPasswordChanged]) => {
if (
((isOtpwUser && !isOptwPasswordChanged) || (isOtpwUser && !userConfig.secret_configured))
&& globalConfig.enabled
) {
return this.showStigFirstLoginDialog();
}

if (!globalConfig.enabled || userConfig.secret_configured || state.url.endsWith('/two-factor-auth')) {
return of(true);
}
Expand All @@ -70,4 +82,17 @@ export class TwoFactorGuardService implements CanActivateChild {
}),
);
}

private showStigFirstLoginDialog(): Observable<boolean> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I would avoid stig in naming here because it's conceivable that this feature may be extended for other scenarios in the future.
  2. How about unifying the flow so that FirstLoginDialogComponent is always shown when it's either otpw user or 2FA setup is needed? And then letting FirstLoginDialogComponent decide which steps to show.

Copy link
Contributor Author

@AlexKarpov98 AlexKarpov98 Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@undsoft
We have such modal as well, if user missed 2fa configuration:

Screenshot 2025-01-29 at 13 41 00

And then navigated to the 2fa setup page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@undsoft - for STIG user, it's impossible to login without one time password or 2fa.
So if I changed password but didn't set up 2fa, I'll see the following.

Screenshot 2025-01-29 at 13 43 31

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've updated this dialog to work for both:
OtpwUser and NonOtpwUser, but it's useless at this point.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that we can get rid of showTwoFactorWarning() and only have one branch. If this is an OTPW user OR if 2FA setup is required for normal user, take them to FirstLoginDialogComponent.
If this is an OTPW user, show password form, if this is a normal user, only show the 2FA part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, got it, probably I need to add more descriptions there as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we request it as a separate ticket? I think it worth a separate ticket since a lot of changes were made already as for this one ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make your code simpler, but okay, please create a ticket then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure 👌

const dialogRef = this.matDialog.open(StigFirstLoginDialogComponent, {
maxWidth: '100vw',
maxHeight: '100vh',
height: '100%',
width: '100%',
panelClass: 'full-screen-modal',
disableClose: true,
});

return dialogRef.afterClosed();
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
@import 'scss-imports/variables';

:host {
color: var(--fg1);
display: block;
font-size: 12px;

margin-bottom: 19px;
margin-top: 12px;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,42 +1,5 @@
<h1 matDialogTitle>{{ 'Change Password' | translate }}</h1>
<form class="ix-form-container" [formGroup]="form" (submit)="onSubmit()">
@if (!(isFullAdminUser$ | async)) {
<ix-input
formControlName="old_password"
type="password"
[label]="'Current Password' | translate"
[required]="true"
></ix-input>
}

<ix-input
formControlName="new_password"
type="password"
[label]="'New Password' | translate"
[required]="true"
[tooltip]="tooltips.password | translate"
></ix-input>

<ix-input
formControlName="passwordConfirmation"
type="password"
[label]="'Confirm Password' | translate"
[required]="true"
></ix-input>

<ix-form-actions>
<button mat-button type="button" matDialogClose ixTest="cancel">
{{ 'Cancel' | translate }}
</button>

<button
mat-button
type="submit"
color="primary"
ixTest="save"
[disabled]="form.invalid"
>
{{ 'Save' | translate }}
</button>
</ix-form-actions>
</form>
<ix-change-password-form
[usedInDialog]="true"
></ix-change-password-form>
Original file line number Diff line number Diff line change
@@ -1,92 +1,28 @@
import { HarnessLoader } from '@angular/cdk/testing';
import { TestbedHarnessEnvironment } from '@angular/cdk/testing/testbed';
import { FormGroup, ReactiveFormsModule } from '@angular/forms';
import { MatButtonHarness } from '@angular/material/button/testing';
import { ReactiveFormsModule } from '@angular/forms';
import { MatDialogRef } from '@angular/material/dialog';
import { createComponentFactory, mockProvider, Spectator } from '@ngneat/spectator/jest';
import { throwError } from 'rxjs';
import { MockAuthService } from 'app/core/testing/classes/mock-auth.service';
import { mockCall, mockApi } from 'app/core/testing/utils/mock-api.utils';
import { mockAuth } from 'app/core/testing/utils/mock-auth.utils';
import { Role } from 'app/enums/role.enum';
import { FormErrorHandlerService } from 'app/modules/forms/ix-forms/services/form-error-handler.service';
import { IxFormHarness } from 'app/modules/forms/ix-forms/testing/ix-form.harness';
import { ChangePasswordDialogComponent } from 'app/modules/layout/topbar/change-password-dialog/change-password-dialog.component';
import { ApiService } from 'app/modules/websocket/api.service';
import { ChangePasswordFormComponent } from 'app/modules/layout/topbar/change-password-dialog/change-password-form/change-password-form.component';

describe('ChangePasswordDialogComponent', () => {
let spectator: Spectator<ChangePasswordDialogComponent>;
let loader: HarnessLoader;
let api: ApiService;

const createComponent = createComponentFactory({
component: ChangePasswordDialogComponent,
imports: [
ReactiveFormsModule,
],
providers: [
mockApi([
mockCall('user.set_password'),
]),
mockProvider(FormErrorHandlerService),
mockProvider(MatDialogRef),
mockAuth(),
],
});

beforeEach(() => {
spectator = createComponent();
loader = TestbedHarnessEnvironment.loader(spectator.fixture);
api = spectator.inject(ApiService);
});

it('does not show current password field for full admin', async () => {
const authMock = spectator.inject(MockAuthService);
authMock.setRoles([Role.FullAdmin]);

const form = await loader.getHarness(IxFormHarness);
expect(await form.getControl('Current Password')).toBeUndefined();
});

it('checks current password, updates to new password and closes the dialog when form is saved', async () => {
const authMock = spectator.inject(MockAuthService);
authMock.setRoles([Role.ReadonlyAdmin]);

const form = await loader.getHarness(IxFormHarness);
await form.fillForm({
'Current Password': 'correct',
'New Password': '123456',
'Confirm Password': '123456',
});

const saveButton = await loader.getHarness(MatButtonHarness.with({ text: 'Save' }));
await saveButton.click();

expect(api.call).toHaveBeenCalledWith('user.set_password', [{
old_password: 'correct',
new_password: '123456',
username: 'root',
}]);
expect(spectator.inject(MatDialogRef).close).toHaveBeenCalled();
});

it('shows error if any happened during password change request', async () => {
const authMock = spectator.inject(MockAuthService);
authMock.setRoles([Role.ReadonlyAdmin]);

const error = new Error('error');
jest.spyOn(api, 'call').mockReturnValue(throwError(() => error));

const form = await loader.getHarness(IxFormHarness);
await form.fillForm({
'Current Password': 'incorrect',
'New Password': '123456',
'Confirm Password': '123456',
});

const saveButton = await loader.getHarness(MatButtonHarness.with({ text: 'Save' }));
await saveButton.click();

expect(spectator.inject(FormErrorHandlerService).handleValidationErrors)
.toHaveBeenCalledWith(error, expect.any(FormGroup));
it('renders the change password form', () => {
const formComponent = spectator.query(ChangePasswordFormComponent);
expect(formComponent).toBeTruthy();
});
});
Loading
Loading