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

PIDP-1011 Linking Accounts Error fix + Business events #593

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using Microsoft.EntityFrameworkCore.Migrations;

#nullable disable

namespace Pidp.Data.Migrations
{
/// <inheritdoc />
public partial class AccountLinkingBusinessEvents : Migration
{
/// <inheritdoc />
protected override void Up(MigrationBuilder migrationBuilder)
{

}

/// <inheritdoc />
protected override void Down(MigrationBuilder migrationBuilder)
{

}
}
}
54 changes: 54 additions & 0 deletions backend/webapi/Data/Migrations/PidpDbContextModelSnapshot.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1200,6 +1200,38 @@ protected override void BuildModel(ModelBuilder modelBuilder)
b.HasDiscriminator().HasValue("MSTeamsClinicAddress");
});

modelBuilder.Entity("Pidp.Models.AccountLinkingFailure", b =>
{
b.HasBaseType("Pidp.Models.BusinessEvent");

b.Property<int>("PartyId")
.ValueGeneratedOnUpdateSometimes()
.HasColumnType("integer")
.HasColumnName("PartyId");

b.HasIndex("PartyId");

b.ToTable("BusinessEvent");

b.HasDiscriminator().HasValue("AccountLinkingFailure");
});

modelBuilder.Entity("Pidp.Models.AccountLinkingSuccess", b =>
{
b.HasBaseType("Pidp.Models.BusinessEvent");

b.Property<int>("PartyId")
.ValueGeneratedOnUpdateSometimes()
.HasColumnType("integer")
.HasColumnName("PartyId");

b.HasIndex("PartyId");

b.ToTable("BusinessEvent");

b.HasDiscriminator().HasValue("AccountLinkingSuccess");
});

modelBuilder.Entity("Pidp.Models.BCProviderPasswordReset", b =>
{
b.HasBaseType("Pidp.Models.BusinessEvent");
Expand Down Expand Up @@ -1437,6 +1469,28 @@ protected override void BuildModel(ModelBuilder modelBuilder)
b.Navigation("Clinic");
});

modelBuilder.Entity("Pidp.Models.AccountLinkingFailure", b =>
{
b.HasOne("Pidp.Models.Party", "Party")
.WithMany()
.HasForeignKey("PartyId")
.OnDelete(DeleteBehavior.Cascade)
.IsRequired();

b.Navigation("Party");
});

modelBuilder.Entity("Pidp.Models.AccountLinkingSuccess", b =>
{
b.HasOne("Pidp.Models.Party", "Party")
.WithMany()
.HasForeignKey("PartyId")
.OnDelete(DeleteBehavior.Cascade)
.IsRequired();

b.Navigation("Party");
});

modelBuilder.Entity("Pidp.Models.BCProviderPasswordReset", b =>
{
b.HasOne("Pidp.Models.Party", "Party")
Expand Down
5 changes: 3 additions & 2 deletions backend/webapi/Features/Credentials/CredentialsController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,12 @@ await this.AuthorizationService.SignTokenAsync(new Cookies.CredentialLinkTicket.
return result.ToActionResult();
}

[HttpDelete("/api/[controller]")]
[HttpDelete("/api/[controller]/link-ticket/cookie")]
[AllowAnonymous]
kakarlavinodkumar marked this conversation as resolved.
Show resolved Hide resolved
[ProducesResponseType(StatusCodes.Status200OK)]
[ProducesResponseType(StatusCodes.Status400BadRequest)]
[ProducesResponseType(StatusCodes.Status404NotFound)]
public ActionResult DeleteCredential()
public ActionResult DeleteCredentialLinkCookie()
{
this.Response.Cookies.Append(
Cookies.CredentialLinkTicket.Key,
Expand Down
14 changes: 13 additions & 1 deletion backend/webapi/Features/Discovery/Discovery.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@

public int? PartyId { get; set; }
public StatusCode Status { get; set; }
public static readonly string TicketNotFound = "TICKET NOT FOUND";
public static readonly string UnexpectedLinkingCredential = "UNEXPECTED LINKING CREDENTIAL";
public static readonly string ExpiredTicket = "EXPIRED TICKET";
}

public class QueryHandler : IQueryHandler<Query, Model>
Expand Down Expand Up @@ -111,16 +114,22 @@
if (ticket == null)
{
this.logger.LogTicketNotFound(query.User.GetUserId(), query.CredentialLinkToken!.Value);
this.context.BusinessEvents.Add(AccountLinkingFailure.Create(credential.PartyId, Model.TicketNotFound, this.clock.GetCurrentInstant()));

Check warning on line 117 in backend/webapi/Features/Discovery/Discovery.cs

View workflow job for this annotation

GitHub Actions / unit-test-backend

Dereference of a possibly null reference.
await this.context.SaveChangesAsync();
return new Model { Status = Model.StatusCode.AccountLinkingError };
}
if (ticket.LinkToIdentityProvider != query.User.GetIdentityProvider())
{
this.logger.LogTicketIdpError(query.User.GetUserId(), ticket.Id, ticket.LinkToIdentityProvider, query.User.GetIdentityProvider());
this.context.BusinessEvents.Add(AccountLinkingFailure.Create(credential.PartyId, Model.UnexpectedLinkingCredential, this.clock.GetCurrentInstant()));
await this.context.SaveChangesAsync();
return new Model { Status = Model.StatusCode.AccountLinkingError };
}
if (ticket.ExpiresAt < this.clock.GetCurrentInstant())
{
this.logger.LogTicketExpired(query.User.GetUserId(), ticket.Id);
this.context.BusinessEvents.Add(AccountLinkingFailure.Create(credential.PartyId, Model.ExpiredTicket, this.clock.GetCurrentInstant()));
await this.context.SaveChangesAsync();
return new Model { Status = Model.StatusCode.TicketExpired };
}

Expand All @@ -135,6 +144,8 @@
if (credential.PartyId == ticket.PartyId)
{
this.logger.LogCredentialAlreadyLinked(query.User.GetUserId(), ticket.Id, credential.Id);
this.context.BusinessEvents.Add(AccountLinkingFailure.Create(credential.PartyId, "CREDENTIAL ALREADY LINKED", this.clock.GetCurrentInstant()));
kakarlavinodkumar marked this conversation as resolved.
Show resolved Hide resolved
await this.context.SaveChangesAsync();
return new Model
{
PartyId = credential.PartyId,
Expand All @@ -148,9 +159,10 @@
CredentialLinkTicketId = ticket.Id,
ExistingCredentialId = credential.Id
});
await this.context.SaveChangesAsync();

this.logger.LogCredentialAlreadyExists(query.User.GetUserId(), ticket.Id, credential.Id);
this.context.BusinessEvents.Add(AccountLinkingFailure.Create(credential.PartyId, "CREDENTIAL EXISTS", this.clock.GetCurrentInstant()));
Paahn marked this conversation as resolved.
Show resolved Hide resolved
await this.context.SaveChangesAsync();
return new Model
{
PartyId = credential.PartyId,
Expand Down
28 changes: 28 additions & 0 deletions backend/webapi/Models/BusinessEvent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,31 @@ public static BCProviderPasswordReset Create(int partyId, string userPrincipalNa
}
}

public class AccountLinkingSuccess : PartyBusinessEvent
{
public static AccountLinkingSuccess Create(int partyId, Instant recordedOn)
{
return new AccountLinkingSuccess
{
PartyId = partyId,
Description = $"Party successfully linked their account.",
Severity = LogLevel.Information,
RecordedOn = recordedOn
};
}
}

public class AccountLinkingFailure : PartyBusinessEvent
{
public static AccountLinkingFailure Create(int partyId, string failureReason, Instant recordedOn)
{
return new AccountLinkingFailure
{
PartyId = partyId,
Description = $"Party failed to link their account. Reason: {failureReason}",
Severity = LogLevel.Warning,
Paahn marked this conversation as resolved.
Show resolved Hide resolved
RecordedOn = recordedOn
};
}
}

Original file line number Diff line number Diff line change
@@ -1,27 +1,19 @@
import { HttpErrorResponse } from '@angular/common/http';
import { Inject, Injectable } from '@angular/core';
import { Injectable } from '@angular/core';

import { Observable, catchError, map, tap, throwError } from 'rxjs';
import { Observable, catchError, tap, throwError } from 'rxjs';

import { APP_CONFIG, AppConfig } from '@app/app.config';
import { ApiHttpClient } from '@app/core/resources/api-http-client.service';
import { ToastService } from '@app/core/services/toast.service';

import { AuthService } from '../../services/auth.service';

@Injectable({
providedIn: 'root',
})
export class LinkAccountConfirmResource {
public logoutRedirectUrl: string;
public constructor(
@Inject(APP_CONFIG) private config: AppConfig,
private apiResource: ApiHttpClient,
private authService: AuthService,
private toastService: ToastService,
) {
this.logoutRedirectUrl = `${this.config.applicationUrl}/`;
}
) {}

public linkAccount(): Observable<number> {
return this.apiResource.post<number>('credentials', {}).pipe(
Expand All @@ -39,9 +31,8 @@ export class LinkAccountConfirmResource {
);
}

public cancelLink(): Observable<Observable<void> | boolean> {
return this.apiResource.delete('credentials').pipe(
map(() => this.authService.logout(this.logoutRedirectUrl)),
public cancelLink(): Observable<unknown> {
Paahn marked this conversation as resolved.
Show resolved Hide resolved
return this.apiResource.delete('credentials/link-ticket/cookie').pipe(
catchError((error: HttpErrorResponse) => {
this.toastService.openErrorToast(
'Something went wrong. Please try again.',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { CommonModule, NgOptimizedImage } from '@angular/common';
import { Component, OnInit } from '@angular/core';
import { Component, Inject, OnInit } from '@angular/core';
import { MatButtonModule } from '@angular/material/button';
import { MatDialog } from '@angular/material/dialog';
import { Router } from '@angular/router';

import { Observable, exhaustMap, switchMap, tap } from 'rxjs';
import { Observable, concatMap, exhaustMap, switchMap, tap } from 'rxjs';

import {
LOADING_OVERLAY_DEFAULT_MESSAGE,
Expand All @@ -18,16 +18,18 @@ import {
InjectViewportCssClassDirective,
} from '@bcgov/shared/ui';

import { APP_CONFIG, AppConfig } from '@app/app.config';
import { AccessRoutes } from '@app/features/access/access.routes';
import { User } from '@app/features/auth/models/user.model';
import { ProfileRoutes } from '@app/features/profile/profile.routes';
import { BreadcrumbComponent } from '@app/shared/components/breadcrumb/breadcrumb.component';
import { SuccessDialogComponent } from '@app/shared/components/success-dialog/success-dialog.component';

import { IdentityProvider } from '../../enums/identity-provider.enum';
import { BcProviderUser } from '../../models/bc-provider-user.model';
import { AuthService } from '../../services/auth.service';
import { AuthorizedUserService } from '../../services/authorized-user.service';
import { LinkAccountConfirmResource } from './link-account-confirm-resource.service';
import { AccessRoutes } from '@app/features/access/access.routes';

@Component({
selector: 'app-link-account-confirm',
Expand All @@ -46,23 +48,31 @@ import { AccessRoutes } from '@app/features/access/access.routes';
export class LinkAccountConfirmPage implements OnInit {
public user$: Observable<User>;
public breadcrumbsData: Array<{ title: string; path: string }> = [
{title: 'Home', path: ''},
{ title: 'Home', path: '' },
{
title: 'Access',
path: AccessRoutes.routePath(AccessRoutes.ACCESS_REQUESTS),
},
{title: 'Link Account', path: ''},
{ title: 'Link Account', path: '' },
];

public showInstructions: boolean = false;
public logoutRedirectUrl: string;
public constructor(
@Inject(APP_CONFIG) private config: AppConfig,
private dialog: MatDialog,
private authorizedUserService: AuthorizedUserService,
private linkAccountConfirmResource: LinkAccountConfirmResource,
private router: Router,
private loadingOverlayService: LoadingOverlayService,
private authService: AuthService,
) {
this.user$ = this.authorizedUserService.user$;
this.logoutRedirectUrl = `${this.config.applicationUrl}/`;
}

public toggleInstructions(): void {
this.showInstructions = !this.showInstructions;
}

public ngOnInit(): void {
Expand Down Expand Up @@ -97,9 +107,7 @@ export class LinkAccountConfirmPage implements OnInit {
.afterClosed()
.pipe(
exhaustMap((result) =>
result
? this.link()
: this.linkAccountConfirmResource.cancelLink(),
result ? this.link() : this.cancelLink(),
),
);
}),
Expand All @@ -119,8 +127,10 @@ export class LinkAccountConfirmPage implements OnInit {
);
}

public toggleInstructions(): void {
this.showInstructions = !this.showInstructions;
private cancelLink(): Observable<void> {
return this.linkAccountConfirmResource
.cancelLink()
.pipe(concatMap(() => this.authService.logout(this.logoutRedirectUrl)));
}

private getPendingAccountDescription(user: User): string {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@ import { DocumentService } from '@app/core/services/document.service';

import { IdentityProvider } from '../../enums/identity-provider.enum';
import { AuthService } from '../../services/auth.service';
import { LinkAccountConfirmResource } from '../link-account-confirm/link-account-confirm-resource.service';
import { LoginPage, LoginPageRouteData } from './login.page';

describe('LoginPage', () => {
let component: LoginPage;

let clientLogsServiceSpy: Spy<ClientLogsService>;
let linkAccountConfirmResourceSpy: Spy<LinkAccountConfirmResource>;
let authServiceSpy: Spy<AuthService>;
let matDialogSpy: Spy<MatDialog>;

Expand Down Expand Up @@ -70,13 +72,17 @@ describe('LoginPage', () => {
provideAutoSpy(ClientLogsService),
provideAutoSpy(DocumentService),
provideAutoSpy(MatDialog),
provideAutoSpy(LinkAccountConfirmResource),
ViewportService,
],
}).compileComponents();

component = TestBed.inject(LoginPage);

clientLogsServiceSpy = TestBed.inject<any>(ClientLogsService);
linkAccountConfirmResourceSpy = TestBed.inject<any>(
LinkAccountConfirmResource,
);
authServiceSpy = TestBed.inject<any>(AuthService);
matDialogSpy = TestBed.inject<any>(MatDialog);
});
Expand Down Expand Up @@ -122,6 +128,7 @@ describe('LoginPage', () => {
});
const idpHint = IdentityProvider.IDIR;
clientLogsServiceSpy.createClientLog.mockReturnValue(of(void 0));
linkAccountConfirmResourceSpy.cancelLink.mockReturnValue(of(void 0));
component.ngOnInit();

when('the method is called', () => {
Expand Down Expand Up @@ -151,6 +158,7 @@ describe('LoginPage', () => {
});
const idpHint = IdentityProvider.BCSC;
clientLogsServiceSpy.createClientLog.mockReturnValue(of(void 0));
linkAccountConfirmResourceSpy.cancelLink.mockReturnValue(of(void 0));
matDialogSpy.open.mockReturnValue({
afterClosed: () => of(true),
} as MatDialogRef<typeof component>);
Expand Down Expand Up @@ -186,6 +194,7 @@ describe('LoginPage', () => {
mockActivatedRoute.snapshot.routeConfig = { path: 'admin' };
const idpHint = IdentityProvider.BCSC;
clientLogsServiceSpy.createClientLog.mockReturnValue(of(void 0));
linkAccountConfirmResourceSpy.cancelLink.mockReturnValue(of(void 0));
matDialogSpy.open.mockReturnValue({
afterClosed: () => of(true),
} as MatDialogRef<typeof component>);
Expand Down
Loading
Loading