From d81eb7ddae5a430f776ab39e296b6dfb526cdfb7 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Wed, 2 Mar 2022 07:31:00 +1000 Subject: [PATCH] Improve SSO Config validation (#572) * Extract SsoConfig enums to own file * Add ChangeStripSpaces directive * Move custom validators to jslib * Add a11y-invalid directive * Add and implement dirtyValidators * Create ssoConfigView model and factory methods * Add interface for select options * Don't build SsoConfigData if null Co-authored-by: Oscar Hinton --- .../src/directives/a11y-invalid.directive.ts | 26 +++++ .../input-strip-spaces.directive.ts | 12 ++ angular/src/interfaces/selectOptions.ts | 5 + angular/src/validators/dirty.validator.ts | 25 ++++ .../src/validators/requiredIf.validator.ts | 10 ++ common/src/enums/ssoEnums.ts | 34 ++++++ common/src/models/api/ssoConfigApi.ts | 84 ++++++++------ .../organization/organizationSsoResponse.ts | 5 +- common/src/models/view/ssoConfigView.ts | 107 ++++++++++++++++++ 9 files changed, 274 insertions(+), 34 deletions(-) create mode 100644 angular/src/directives/a11y-invalid.directive.ts create mode 100644 angular/src/directives/input-strip-spaces.directive.ts create mode 100644 angular/src/interfaces/selectOptions.ts create mode 100644 angular/src/validators/dirty.validator.ts create mode 100644 angular/src/validators/requiredIf.validator.ts create mode 100644 common/src/enums/ssoEnums.ts create mode 100644 common/src/models/view/ssoConfigView.ts diff --git a/angular/src/directives/a11y-invalid.directive.ts b/angular/src/directives/a11y-invalid.directive.ts new file mode 100644 index 0000000000..c5f0e40658 --- /dev/null +++ b/angular/src/directives/a11y-invalid.directive.ts @@ -0,0 +1,26 @@ +import { Directive, ElementRef, OnDestroy, OnInit } from "@angular/core"; +import { NgControl } from "@angular/forms"; +import { Subscription } from "rxjs"; + +@Directive({ + selector: "[appA11yInvalid]", +}) +export class A11yInvalidDirective implements OnDestroy, OnInit { + private sub: Subscription; + + constructor(private el: ElementRef, private formControlDirective: NgControl) {} + + ngOnInit() { + this.sub = this.formControlDirective.control.statusChanges.subscribe((status) => { + if (status === "INVALID") { + this.el.nativeElement.setAttribute("aria-invalid", "true"); + } else if (status === "VALID") { + this.el.nativeElement.setAttribute("aria-invalid", "false"); + } + }); + } + + ngOnDestroy() { + this.sub?.unsubscribe(); + } +} diff --git a/angular/src/directives/input-strip-spaces.directive.ts b/angular/src/directives/input-strip-spaces.directive.ts new file mode 100644 index 0000000000..598281b85c --- /dev/null +++ b/angular/src/directives/input-strip-spaces.directive.ts @@ -0,0 +1,12 @@ +import { Directive, ElementRef, HostListener } from "@angular/core"; + +@Directive({ + selector: "input[appInputStripSpaces]", +}) +export class InputStripSpacesDirective { + constructor(private el: ElementRef) {} + + @HostListener("input") onInput() { + this.el.nativeElement.value = this.el.nativeElement.value.replace(/ /g, ""); + } +} diff --git a/angular/src/interfaces/selectOptions.ts b/angular/src/interfaces/selectOptions.ts new file mode 100644 index 0000000000..19da14d9f3 --- /dev/null +++ b/angular/src/interfaces/selectOptions.ts @@ -0,0 +1,5 @@ +export interface SelectOptions { + name: string; + value: any; + disabled?: boolean; +} diff --git a/angular/src/validators/dirty.validator.ts b/angular/src/validators/dirty.validator.ts new file mode 100644 index 0000000000..14875631ee --- /dev/null +++ b/angular/src/validators/dirty.validator.ts @@ -0,0 +1,25 @@ +import { AbstractControl, ValidationErrors, ValidatorFn, Validators } from "@angular/forms"; +import { requiredIf } from "./requiredIf.validator"; + +/** + * A higher order function that takes a ValidatorFn and returns a new validator. + * The new validator only runs the ValidatorFn if the control is dirty. This prevents error messages from being + * displayed to the user prematurely. + */ +function dirtyValidator(validator: ValidatorFn) { + return (control: AbstractControl): ValidationErrors | null => { + return control.dirty ? validator(control) : null; + }; +} + +export function dirtyRequiredIf(predicate: (predicateCtrl: AbstractControl) => boolean) { + return dirtyValidator(requiredIf(predicate)); +} + +/** + * Equivalent to dirtyValidator(Validator.required), however using dirtyValidator returns a new function + * each time which prevents formControl.hasError from properly comparing functions for equality. + */ +export function dirtyRequired(control: AbstractControl): ValidationErrors | null { + return control.dirty ? Validators.required(control) : null; +} diff --git a/angular/src/validators/requiredIf.validator.ts b/angular/src/validators/requiredIf.validator.ts new file mode 100644 index 0000000000..50d5ecf702 --- /dev/null +++ b/angular/src/validators/requiredIf.validator.ts @@ -0,0 +1,10 @@ +import { AbstractControl, ValidationErrors, Validators } from "@angular/forms"; + +/** + * Returns a new validator which will apply Validators.required only if the predicate is true. + */ +export function requiredIf(predicate: (predicateCtrl: AbstractControl) => boolean) { + return (control: AbstractControl): ValidationErrors | null => { + return predicate(control) ? Validators.required(control) : null; + }; +} diff --git a/common/src/enums/ssoEnums.ts b/common/src/enums/ssoEnums.ts new file mode 100644 index 0000000000..fdf9c0b73f --- /dev/null +++ b/common/src/enums/ssoEnums.ts @@ -0,0 +1,34 @@ +export enum SsoType { + None = 0, + OpenIdConnect = 1, + Saml2 = 2, +} + +export enum OpenIdConnectRedirectBehavior { + RedirectGet = 0, + FormPost = 1, +} + +export enum Saml2BindingType { + HttpRedirect = 1, + HttpPost = 2, + Artifact = 4, +} + +export enum Saml2NameIdFormat { + NotConfigured = 0, + Unspecified = 1, + EmailAddress = 2, + X509SubjectName = 3, + WindowsDomainQualifiedName = 4, + KerberosPrincipalName = 5, + EntityIdentifier = 6, + Persistent = 7, + Transient = 8, +} + +export enum Saml2SigningBehavior { + IfIdpWantAuthnRequestsSigned = 0, + Always = 1, + Never = 3, +} diff --git a/common/src/models/api/ssoConfigApi.ts b/common/src/models/api/ssoConfigApi.ts index 338ab79357..c6a2456d85 100644 --- a/common/src/models/api/ssoConfigApi.ts +++ b/common/src/models/api/ssoConfigApi.ts @@ -1,40 +1,58 @@ import { BaseResponse } from "../response/baseResponse"; -enum SsoType { - OpenIdConnect = 1, - Saml2 = 2, -} - -enum OpenIdConnectRedirectBehavior { - RedirectGet = 0, - FormPost = 1, -} - -enum Saml2BindingType { - HttpRedirect = 1, - HttpPost = 2, - Artifact = 4, -} - -enum Saml2NameIdFormat { - NotConfigured = 0, - Unspecified = 1, - EmailAddress = 2, - X509SubjectName = 3, - WindowsDomainQualifiedName = 4, - KerberosPrincipalName = 5, - EntityIdentifier = 6, - Persistent = 7, - Transient = 8, -} - -enum Saml2SigningBehavior { - IfIdpWantAuthnRequestsSigned = 0, - Always = 1, - Never = 3, -} +import { + OpenIdConnectRedirectBehavior, + Saml2BindingType, + Saml2NameIdFormat, + Saml2SigningBehavior, + SsoType, +} from "../../enums/ssoEnums"; +import { SsoConfigView } from "../view/ssoConfigView"; export class SsoConfigApi extends BaseResponse { + static fromView(view: SsoConfigView, api = new SsoConfigApi()) { + api.configType = view.configType; + + api.keyConnectorEnabled = view.keyConnectorEnabled; + api.keyConnectorUrl = view.keyConnectorUrl; + + if (api.configType === SsoType.OpenIdConnect) { + api.authority = view.openId.authority; + api.clientId = view.openId.clientId; + api.clientSecret = view.openId.clientSecret; + api.metadataAddress = view.openId.metadataAddress; + api.redirectBehavior = view.openId.redirectBehavior; + api.getClaimsFromUserInfoEndpoint = view.openId.getClaimsFromUserInfoEndpoint; + api.additionalScopes = view.openId.additionalScopes; + api.additionalUserIdClaimTypes = view.openId.additionalUserIdClaimTypes; + api.additionalEmailClaimTypes = view.openId.additionalEmailClaimTypes; + api.additionalNameClaimTypes = view.openId.additionalNameClaimTypes; + api.acrValues = view.openId.acrValues; + api.expectedReturnAcrValue = view.openId.expectedReturnAcrValue; + } else if (api.configType === SsoType.Saml2) { + api.spNameIdFormat = view.saml.spNameIdFormat; + api.spOutboundSigningAlgorithm = view.saml.spOutboundSigningAlgorithm; + api.spSigningBehavior = view.saml.spSigningBehavior; + api.spMinIncomingSigningAlgorithm = view.saml.spMinIncomingSigningAlgorithm; + api.spWantAssertionsSigned = view.saml.spWantAssertionsSigned; + api.spValidateCertificates = view.saml.spValidateCertificates; + + api.idpEntityId = view.saml.idpEntityId; + api.idpBindingType = view.saml.idpBindingType; + api.idpSingleSignOnServiceUrl = view.saml.idpSingleSignOnServiceUrl; + api.idpSingleLogoutServiceUrl = view.saml.idpSingleLogoutServiceUrl; + api.idpArtifactResolutionServiceUrl = view.saml.idpArtifactResolutionServiceUrl; + api.idpX509PublicCert = view.saml.idpX509PublicCert; + api.idpOutboundSigningAlgorithm = view.saml.idpOutboundSigningAlgorithm; + api.idpAllowUnsolicitedAuthnResponse = view.saml.idpAllowUnsolicitedAuthnResponse; + api.idpWantAuthnRequestsSigned = view.saml.idpWantAuthnRequestsSigned; + + // Value is inverted in the api model (disable instead of allow) + api.idpDisableOutboundLogoutRequests = !view.saml.idpAllowOutboundLogoutRequests; + } + + return api; + } configType: SsoType; keyConnectorEnabled: boolean; diff --git a/common/src/models/response/organization/organizationSsoResponse.ts b/common/src/models/response/organization/organizationSsoResponse.ts index 7143291206..abe6b72063 100644 --- a/common/src/models/response/organization/organizationSsoResponse.ts +++ b/common/src/models/response/organization/organizationSsoResponse.ts @@ -9,7 +9,10 @@ export class OrganizationSsoResponse extends BaseResponse { constructor(response: any) { super(response); this.enabled = this.getResponseProperty("Enabled"); - this.data = new SsoConfigApi(this.getResponseProperty("Data")); + this.data = + this.getResponseProperty("Data") != null + ? new SsoConfigApi(this.getResponseProperty("Data")) + : null; this.urls = new SsoUrls(this.getResponseProperty("Urls")); } } diff --git a/common/src/models/view/ssoConfigView.ts b/common/src/models/view/ssoConfigView.ts new file mode 100644 index 0000000000..1fff053254 --- /dev/null +++ b/common/src/models/view/ssoConfigView.ts @@ -0,0 +1,107 @@ +import { View } from "./view"; + +import { SsoConfigApi } from "../api/ssoConfigApi"; + +import { + OpenIdConnectRedirectBehavior, + Saml2BindingType, + Saml2NameIdFormat, + Saml2SigningBehavior, + SsoType, +} from "../../enums/ssoEnums"; + +export class SsoConfigView extends View { + configType: SsoType; + + keyConnectorEnabled: boolean; + keyConnectorUrl: string; + + openId: { + authority: string; + clientId: string; + clientSecret: string; + metadataAddress: string; + redirectBehavior: OpenIdConnectRedirectBehavior; + getClaimsFromUserInfoEndpoint: boolean; + additionalScopes: string; + additionalUserIdClaimTypes: string; + additionalEmailClaimTypes: string; + additionalNameClaimTypes: string; + acrValues: string; + expectedReturnAcrValue: string; + }; + + saml: { + spNameIdFormat: Saml2NameIdFormat; + spOutboundSigningAlgorithm: string; + spSigningBehavior: Saml2SigningBehavior; + spMinIncomingSigningAlgorithm: boolean; + spWantAssertionsSigned: boolean; + spValidateCertificates: boolean; + + idpEntityId: string; + idpBindingType: Saml2BindingType; + idpSingleSignOnServiceUrl: string; + idpSingleLogoutServiceUrl: string; + idpArtifactResolutionServiceUrl: string; + idpX509PublicCert: string; + idpOutboundSigningAlgorithm: string; + idpAllowUnsolicitedAuthnResponse: boolean; + idpAllowOutboundLogoutRequests: boolean; + idpWantAuthnRequestsSigned: boolean; + }; + + constructor(api: SsoConfigApi) { + super(); + if (api == null) { + return; + } + + this.configType = api.configType; + + this.keyConnectorEnabled = api.keyConnectorEnabled; + this.keyConnectorUrl = api.keyConnectorUrl; + + if (this.configType === SsoType.OpenIdConnect) { + this.openId = { + authority: api.authority, + clientId: api.clientId, + clientSecret: api.clientSecret, + metadataAddress: api.metadataAddress, + redirectBehavior: api.redirectBehavior, + getClaimsFromUserInfoEndpoint: api.getClaimsFromUserInfoEndpoint, + additionalScopes: api.additionalScopes, + additionalUserIdClaimTypes: api.additionalUserIdClaimTypes, + additionalEmailClaimTypes: api.additionalEmailClaimTypes, + additionalNameClaimTypes: api.additionalNameClaimTypes, + acrValues: api.acrValues, + expectedReturnAcrValue: api.expectedReturnAcrValue, + }; + } else if (this.configType === SsoType.Saml2) { + this.saml = { + spNameIdFormat: api.spNameIdFormat, + spOutboundSigningAlgorithm: api.spOutboundSigningAlgorithm, + spSigningBehavior: api.spSigningBehavior, + spMinIncomingSigningAlgorithm: api.spMinIncomingSigningAlgorithm, + spWantAssertionsSigned: api.spWantAssertionsSigned, + spValidateCertificates: api.spValidateCertificates, + + idpEntityId: api.idpEntityId, + idpBindingType: api.idpBindingType, + idpSingleSignOnServiceUrl: api.idpSingleSignOnServiceUrl, + idpSingleLogoutServiceUrl: api.idpSingleLogoutServiceUrl, + idpArtifactResolutionServiceUrl: api.idpArtifactResolutionServiceUrl, + idpX509PublicCert: api.idpX509PublicCert, + idpOutboundSigningAlgorithm: api.idpOutboundSigningAlgorithm, + idpAllowUnsolicitedAuthnResponse: api.idpAllowUnsolicitedAuthnResponse, + idpWantAuthnRequestsSigned: api.idpWantAuthnRequestsSigned, + + // Value is inverted in the view model (allow instead of disable) + idpAllowOutboundLogoutRequests: + api.idpDisableOutboundLogoutRequests == null + ? null + : !api.idpDisableOutboundLogoutRequests, + }; + } + } +}