From c5fc166211bf0e4cea2de936c84b5fe83894c75e Mon Sep 17 00:00:00 2001 From: Sabrina Date: Fri, 6 Dec 2024 11:41:35 +0000 Subject: [PATCH 1/4] Add validation check for at signs in username --- .../features/login/login.component.spec.ts | 117 ++++++++++++++++-- .../src/app/features/login/login.component.ts | 51 +++++++- 2 files changed, 157 insertions(+), 11 deletions(-) diff --git a/frontend/src/app/features/login/login.component.spec.ts b/frontend/src/app/features/login/login.component.spec.ts index d41d0a7544..c89fea2f91 100644 --- a/frontend/src/app/features/login/login.component.spec.ts +++ b/frontend/src/app/features/login/login.component.spec.ts @@ -9,17 +9,19 @@ import { MockAuthService } from '@core/test-utils/MockAuthService'; import { MockUserService } from '@core/test-utils/MockUserService'; import { FeatureFlagsService } from '@shared/services/feature-flags.service'; import { SharedModule } from '@shared/shared.module'; -import { render } from '@testing-library/angular'; +import { fireEvent, getByTestId, render, within } from '@testing-library/angular'; import { throwError } from 'rxjs'; import { LoginComponent } from './login.component'; +import { ReactiveFormsModule, UntypedFormBuilder } from '@angular/forms'; -describe('LoginComponent', () => { +fdescribe('LoginComponent', () => { async function setup(isAdmin = false, employerTypeSet = true, isAuthenticated = true) { - const { fixture, getAllByText, getByText, queryByText, getByTestId } = await render(LoginComponent, { - imports: [SharedModule, RouterModule, RouterTestingModule, HttpClientTestingModule], + const setupTools = await render(LoginComponent, { + imports: [SharedModule, RouterModule, RouterTestingModule, HttpClientTestingModule, ReactiveFormsModule], providers: [ FeatureFlagsService, + UntypedFormBuilder, { provide: AuthService, useFactory: MockAuthService.factory(true, isAdmin, employerTypeSet), @@ -53,14 +55,13 @@ describe('LoginComponent', () => { authSpy = spyOn(authService, 'authenticate').and.returnValue(throwError(mockErrorResponse)); } + const fixture = setupTools.fixture; const component = fixture.componentInstance; + return { component, fixture, - getAllByText, - getByText, - queryByText, - getByTestId, + ...setupTools, spy, authSpy, }; @@ -71,6 +72,85 @@ describe('LoginComponent', () => { expect(component).toBeTruthy(); }); + describe('username', () => { + it('should show the username hint', async () => { + const { component, getByTestId } = await setup(); + + const usernameHint = getByTestId('username-hint'); + const hintText = 'You cannot use an email address to sign in'; + + expect(within(usernameHint).getByText(hintText)).toBeTruthy(); + }); + }); + + describe('password', () => { + it('should show the password as password field when show password is false', async () => { + const { component, fixture, getByTestId } = await setup(); + + component.showPassword = false; + + fixture.detectChanges(); + + const passwordInput = getByTestId('password'); + + expect(passwordInput.getAttribute('type')).toEqual('password'); + }); + + it('should show the password as text field when show password is true', async () => { + const { component, fixture, getByTestId } = await setup(); + + component.showPassword = true; + + fixture.detectChanges(); + + const passwordInput = getByTestId('password'); + + expect(passwordInput.getAttribute('type')).toEqual('text'); + }); + + it("should initially show 'Show password' text for the password toggle", async () => { + const { component, getByTestId } = await setup(); + + const passwordToggle = getByTestId('password-toggle'); + const toggleText = 'Show password'; + + expect(within(passwordToggle).getByText(toggleText)).toBeTruthy(); + }); + + it("should show 'Hide password' text for the password toggle when 'Show password' is clicked", async () => { + const { component, fixture, getByTestId, getByText } = await setup(); + + const passwordToggle = getByTestId('password-toggle'); + const showToggleText = 'Show password'; + const hideToggleText = 'Hide password'; + + fireEvent.click(getByText(showToggleText)); + fixture.detectChanges(); + + expect(within(passwordToggle).getByText(hideToggleText)).toBeTruthy(); + }); + }); + + it('should show the link to forgot username or password', async () => { + const { component, getByTestId } = await setup(); + + const forgotUsernamePasswordText = 'Forgot your username or password?'; + const forgotUsernamePasswordLink = getByTestId('forgot-username-password'); + + expect(within(forgotUsernamePasswordLink).getByText(forgotUsernamePasswordText)).toBeTruthy(); + expect(forgotUsernamePasswordLink.getAttribute('href')).toEqual('/forgot-your-username-or-password'); + }); + + it('should show the link to create an account', async () => { + const { component, getByTestId } = await setup(); + + const createAccountText = 'Create an account'; + const createAccountLink = getByTestId('create-account'); + + expect(within(createAccountLink).getByText(createAccountText)).toBeTruthy(); + expect(createAccountLink.getAttribute('href')).toEqual('/registration/create-account'); + }); + it('should send you to dashboard on login as user', async () => { const { component, fixture, spy, authSpy } = await setup(); @@ -165,5 +245,26 @@ describe('LoginComponent', () => { fixture.detectChanges(); expect(getAllByText('Your username or your password is incorrect')).toBeTruthy(); }); + + it('should not let you sign in with a username with special characters', async () => { + const { component, fixture, getAllByText, getByTestId } = await setup(); + + const signInButton = within(getByTestId('signinButton')).getByText('Sign in'); + const form = component.form; + + component.form.markAsDirty(); + form.controls['username'].setValue('username@123.com'); + form.controls['username'].markAsDirty(); + component.form.get('password').setValue('1'); + component.form.get('password').markAsDirty(); + + fireEvent.click(signInButton); + fixture.detectChanges(); + + expect(form.invalid).toBeTruthy(); + expect( + getAllByText("You've entered an @ symbol (remember, your username cannot be an email address)"), + ).toBeTruthy(); + }); }); }); diff --git a/frontend/src/app/features/login/login.component.ts b/frontend/src/app/features/login/login.component.ts index e0e48e232c..a5cfd1f773 100644 --- a/frontend/src/app/features/login/login.component.ts +++ b/frontend/src/app/features/login/login.component.ts @@ -1,6 +1,13 @@ import { HttpErrorResponse } from '@angular/common/http'; import { AfterViewInit, Component, ElementRef, OnDestroy, OnInit, ViewChild } from '@angular/core'; -import { UntypedFormBuilder, UntypedFormGroup, Validators } from '@angular/forms'; +import { + AbstractControl, + UntypedFormBuilder, + UntypedFormGroup, + ValidationErrors, + ValidatorFn, + Validators, +} from '@angular/forms'; import { Router } from '@angular/router'; import { ErrorDefinition, ErrorDetails } from '@core/model/errorSummary.model'; import { AuthService } from '@core/services/auth.service'; @@ -14,6 +21,7 @@ import { Subscription } from 'rxjs'; @Component({ selector: 'app-login', templateUrl: './login.component.html', + styleUrls: ['./login.component.scss'], }) export class LoginComponent implements OnInit, OnDestroy, AfterViewInit { @ViewChild('formEl') formEl: ElementRef; @@ -23,6 +31,8 @@ export class LoginComponent implements OnInit, OnDestroy, AfterViewInit { public formErrorsMap: Array; public serverErrorsMap: Array; public serverError: string; + public showPassword: boolean = false; + public regex = "^[A-Za-z0-9._'%+-]*$"; constructor( private idleService: IdleService, @@ -36,8 +46,20 @@ export class LoginComponent implements OnInit, OnDestroy, AfterViewInit { ngOnInit() { this.form = this.formBuilder.group({ - username: [null, { validators: [Validators.required], updateOn: 'submit' }], - password: [null, { validators: [Validators.required], updateOn: 'submit' }], + username: [ + null, + { + validators: [Validators.required, this.checkAtSignInUsername()], + updateOn: 'submit', + }, + ], + password: [ + null, + { + validators: [Validators.required], + updateOn: 'submit', + }, + ], }); this.setupFormErrorsMap(); @@ -52,6 +74,20 @@ export class LoginComponent implements OnInit, OnDestroy, AfterViewInit { this.subscriptions.unsubscribe(); } + public checkAtSignInUsername(): ValidatorFn { + return (control: AbstractControl): ValidationErrors | null => { + const value = control.value; + + if (!value) { + return null; + } + + const userNameHasAtSign = /@/.test(value); + + return userNameHasAtSign ? { isUsernameNotEmail: true } : null; + }; + } + public setupFormErrorsMap(): void { this.formErrorsMap = [ { @@ -61,6 +97,10 @@ export class LoginComponent implements OnInit, OnDestroy, AfterViewInit { name: 'required', message: 'Enter your username', }, + { + name: 'isUsernameNotEmail', + message: "You've entered an @ symbol (remember, your username cannot be an email address)", + }, ], }, { @@ -154,4 +194,9 @@ export class LoginComponent implements OnInit, OnDestroy, AfterViewInit { ), ); } + + public setShowPassword(event: Event): void { + event.preventDefault(); + this.showPassword = !this.showPassword; + } } From 53c8e2458465c5f844856ef5efad07b426658f88 Mon Sep 17 00:00:00 2001 From: Sabrina Date: Fri, 6 Dec 2024 12:59:38 +0000 Subject: [PATCH 2/4] Add password toggle and showadditional error message --- .../app/features/login/login.component.html | 40 ++++++++++++++++--- .../app/features/login/login.component.scss | 5 +++ .../features/login/login.component.spec.ts | 20 +++++----- .../src/app/features/login/login.component.ts | 8 ++-- 4 files changed, 53 insertions(+), 20 deletions(-) create mode 100644 frontend/src/app/features/login/login.component.scss diff --git a/frontend/src/app/features/login/login.component.html b/frontend/src/app/features/login/login.component.html index 4a0117e46d..16db339ac8 100644 --- a/frontend/src/app/features/login/login.component.html +++ b/frontend/src/app/features/login/login.component.html @@ -17,14 +17,25 @@

Sign in

[class.govuk-form-group--error]="(form.get('username').errors || serverError) && submitted" > +
+ You cannot use an email address to sign in +
+ - Error: - {{ getFormErrorMessage('username', 'required') }} + + Error: + {{ getFormErrorMessage('username', 'required') }} + + + + Error: + {{ getFormErrorMessage('username', 'atSignInUsername') }} + Sign in > Error: {{ getFormErrorMessage('password', 'required') }} + + {{ showPassword ? 'Hide' : 'Show' }} password - + diff --git a/frontend/src/app/features/login/login.component.scss b/frontend/src/app/features/login/login.component.scss new file mode 100644 index 0000000000..faf12e9c51 --- /dev/null +++ b/frontend/src/app/features/login/login.component.scss @@ -0,0 +1,5 @@ +@import 'govuk-frontend/govuk/base'; + +.asc-colour-black { + color: $govuk-text-colour; +} diff --git a/frontend/src/app/features/login/login.component.spec.ts b/frontend/src/app/features/login/login.component.spec.ts index c89fea2f91..4fe0de1d70 100644 --- a/frontend/src/app/features/login/login.component.spec.ts +++ b/frontend/src/app/features/login/login.component.spec.ts @@ -9,13 +9,13 @@ import { MockAuthService } from '@core/test-utils/MockAuthService'; import { MockUserService } from '@core/test-utils/MockUserService'; import { FeatureFlagsService } from '@shared/services/feature-flags.service'; import { SharedModule } from '@shared/shared.module'; -import { fireEvent, getByTestId, render, within } from '@testing-library/angular'; +import { fireEvent, render, within } from '@testing-library/angular'; import { throwError } from 'rxjs'; import { LoginComponent } from './login.component'; import { ReactiveFormsModule, UntypedFormBuilder } from '@angular/forms'; -fdescribe('LoginComponent', () => { +describe('LoginComponent', () => { async function setup(isAdmin = false, employerTypeSet = true, isAuthenticated = true) { const setupTools = await render(LoginComponent, { imports: [SharedModule, RouterModule, RouterTestingModule, HttpClientTestingModule, ReactiveFormsModule], @@ -40,7 +40,7 @@ fdescribe('LoginComponent', () => { spy.and.returnValue(Promise.resolve(true)); const authService = injector.inject(AuthService) as AuthService; - let authSpy; + let authSpy: any; if (isAuthenticated) { authSpy = spyOn(authService, 'authenticate'); authSpy.and.callThrough(); @@ -74,7 +74,7 @@ fdescribe('LoginComponent', () => { describe('username', () => { it('should show the username hint', async () => { - const { component, getByTestId } = await setup(); + const { getByTestId } = await setup(); const usernameHint = getByTestId('username-hint'); const hintText = 'You cannot use an email address to sign in'; @@ -109,7 +109,7 @@ fdescribe('LoginComponent', () => { }); it("should initially show 'Show password' text for the password toggle", async () => { - const { component, getByTestId } = await setup(); + const { getByTestId } = await setup(); const passwordToggle = getByTestId('password-toggle'); const toggleText = 'Show password'; @@ -118,7 +118,7 @@ fdescribe('LoginComponent', () => { }); it("should show 'Hide password' text for the password toggle when 'Show password' is clicked", async () => { - const { component, fixture, getByTestId, getByText } = await setup(); + const { fixture, getByTestId, getByText } = await setup(); const passwordToggle = getByTestId('password-toggle'); const showToggleText = 'Show password'; @@ -132,7 +132,7 @@ fdescribe('LoginComponent', () => { }); it('should show the link to forgot username or password', async () => { - const { component, getByTestId } = await setup(); + const { getByTestId } = await setup(); const forgotUsernamePasswordText = 'Forgot your username or password?'; const forgotUsernamePasswordLink = getByTestId('forgot-username-password'); @@ -142,7 +142,7 @@ fdescribe('LoginComponent', () => { }); it('should show the link to create an account', async () => { - const { component, getByTestId } = await setup(); + const { getByTestId } = await setup(); const createAccountText = 'Create an account'; const createAccountLink = getByTestId('create-account'); @@ -263,8 +263,8 @@ fdescribe('LoginComponent', () => { expect(form.invalid).toBeTruthy(); expect( - getAllByText("You've entered an @ symbol (remember, your username cannot be an email address)"), - ).toBeTruthy(); + getAllByText("You've entered an @ symbol (remember, your username cannot be an email address)").length, + ).toBe(2); }); }); }); diff --git a/frontend/src/app/features/login/login.component.ts b/frontend/src/app/features/login/login.component.ts index a5cfd1f773..d820415974 100644 --- a/frontend/src/app/features/login/login.component.ts +++ b/frontend/src/app/features/login/login.component.ts @@ -49,7 +49,7 @@ export class LoginComponent implements OnInit, OnDestroy, AfterViewInit { username: [ null, { - validators: [Validators.required, this.checkAtSignInUsername()], + validators: [Validators.required, this.checkUsernameForAtSign()], updateOn: 'submit', }, ], @@ -74,7 +74,7 @@ export class LoginComponent implements OnInit, OnDestroy, AfterViewInit { this.subscriptions.unsubscribe(); } - public checkAtSignInUsername(): ValidatorFn { + public checkUsernameForAtSign(): ValidatorFn { return (control: AbstractControl): ValidationErrors | null => { const value = control.value; @@ -84,7 +84,7 @@ export class LoginComponent implements OnInit, OnDestroy, AfterViewInit { const userNameHasAtSign = /@/.test(value); - return userNameHasAtSign ? { isUsernameNotEmail: true } : null; + return userNameHasAtSign ? { atSignInUsername: true } : null; }; } @@ -98,7 +98,7 @@ export class LoginComponent implements OnInit, OnDestroy, AfterViewInit { message: 'Enter your username', }, { - name: 'isUsernameNotEmail', + name: 'atSignInUsername', message: "You've entered an @ symbol (remember, your username cannot be an email address)", }, ], From fed95edafa366652e502063ba22a9900a97d0807 Mon Sep 17 00:00:00 2001 From: Sabrina Date: Mon, 9 Dec 2024 09:44:22 +0000 Subject: [PATCH 3/4] Remove unused regex --- .../src/app/features/login/login.component.spec.ts | 11 ++++------- frontend/src/app/features/login/login.component.ts | 1 - 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/frontend/src/app/features/login/login.component.spec.ts b/frontend/src/app/features/login/login.component.spec.ts index 4fe0de1d70..4bf0ff064c 100644 --- a/frontend/src/app/features/login/login.component.spec.ts +++ b/frontend/src/app/features/login/login.component.spec.ts @@ -85,11 +85,7 @@ describe('LoginComponent', () => { describe('password', () => { it('should show the password as password field when show password is false', async () => { - const { component, fixture, getByTestId } = await setup(); - - component.showPassword = false; - - fixture.detectChanges(); + const { getByTestId } = await setup(); const passwordInput = getByTestId('password'); @@ -97,10 +93,11 @@ describe('LoginComponent', () => { }); it('should show the password as text field when show password is true', async () => { - const { component, fixture, getByTestId } = await setup(); + const { fixture, getByTestId, getByText } = await setup(); - component.showPassword = true; + const showToggleText = 'Show password'; + fireEvent.click(getByText(showToggleText)); fixture.detectChanges(); const passwordInput = getByTestId('password'); diff --git a/frontend/src/app/features/login/login.component.ts b/frontend/src/app/features/login/login.component.ts index d820415974..902e06924b 100644 --- a/frontend/src/app/features/login/login.component.ts +++ b/frontend/src/app/features/login/login.component.ts @@ -32,7 +32,6 @@ export class LoginComponent implements OnInit, OnDestroy, AfterViewInit { public serverErrorsMap: Array; public serverError: string; public showPassword: boolean = false; - public regex = "^[A-Za-z0-9._'%+-]*$"; constructor( private idleService: IdleService, From 545e7c945f23c73489d68d49ff0e29b1ab5b4828 Mon Sep 17 00:00:00 2001 From: Sabrina Date: Mon, 9 Dec 2024 09:59:26 +0000 Subject: [PATCH 4/4] Update test names --- frontend/src/app/features/login/login.component.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/app/features/login/login.component.spec.ts b/frontend/src/app/features/login/login.component.spec.ts index 4bf0ff064c..fb46fc7e26 100644 --- a/frontend/src/app/features/login/login.component.spec.ts +++ b/frontend/src/app/features/login/login.component.spec.ts @@ -84,7 +84,7 @@ describe('LoginComponent', () => { }); describe('password', () => { - it('should show the password as password field when show password is false', async () => { + it('should set the password as password field (to hide input) on page load', async () => { const { getByTestId } = await setup(); const passwordInput = getByTestId('password'); @@ -92,7 +92,7 @@ describe('LoginComponent', () => { expect(passwordInput.getAttribute('type')).toEqual('password'); }); - it('should show the password as text field when show password is true', async () => { + it("should show the password as text field after user clicks 'Show password'", async () => { const { fixture, getByTestId, getByText } = await setup(); const showToggleText = 'Show password';