Defect/SG-1083 - Fix SSO Form Validation (#4791)

* SG-1083 - Refactor SSO form validation to work per EC requirements

* Move SSO component into its own folder for better folder management for future components in auth.

* Defect SG-1086 - Domain verification table: Change domain name from anchor tag to button + add title

* SG-1083 - Send null instead of empty string for sso identifier to avoid duplicate key in database issues.

* SG-1086 - Add button type to domain verification button to pass lint rules.

apps/web/src/locales/en/messages.json

@@ -6146,6 +6146,9 @@
   "lastChecked": {
     "message": "Last checked"
   },
+  "editDomain": {
+    "message": "Edit domain"
+  },
   "domainFormInvalid": {
     "message": "There are form errors that need your attention"
   },
@@ -6395,5 +6398,8 @@
   },
   "errorReadingImportFile": {
     "message": "An error occurred when trying to read the import file"
+  },
+  "selectionIsRequired": {
+    "message": "Selection is required."
   }
 }

bitwarden_license/bit-web/src/app/auth/sso/sso-type.validator.ts

@@ -0,0 +1,19 @@
+import { AbstractControl, ValidationErrors, ValidatorFn } from "@angular/forms";
+
+import { SsoType } from "@bitwarden/common/auth/enums/sso";
+
+export function ssoTypeValidator(errorMessage: string): ValidatorFn {
+  return (control: AbstractControl): ValidationErrors | null => {
+    const value = control.value;
+
+    if (value === SsoType.None) {
+      return {
+        validSsoTypeRequired: {
+          message: errorMessage,
+        },
+      };
+    }
+
+    return null;
+  };
+}

bitwarden_license/bit-web/src/app/auth/sso/sso.component.html

@@ -30,14 +30,14 @@
   <ng-container>
     <app-input-checkbox
       controlId="enabled"
-      [formControl]="enabled"
+      formControlName="enabled"
       [label]="'allowSso' | i18n"
       [helperText]="'allowSsoDesc' | i18n"
     ></app-input-checkbox>
 
     <bit-form-field>
       <bit-label>{{ "ssoIdentifier" | i18n }}</bit-label>
-      <input bitInput type="text" [formControl]="ssoIdentifier" />
+      <input bitInput type="text" formControlName="ssoIdentifier" />
       <bit-hint>{{ "ssoIdentifierHint" | i18n }}</bit-hint>
     </bit-form-field>
 

bitwarden_license/bit-web/src/app/auth/sso/sso.component.ts

@@ -30,6 +30,8 @@ import { SsoConfigView } from "@bitwarden/common/auth/models/view/sso-config.vie
 import { Utils } from "@bitwarden/common/misc/utils";
 import { Organization } from "@bitwarden/common/models/domain/organization";
 
+import { ssoTypeValidator } from "./sso-type.validator";
+
 const defaultSigningAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256";
 
 @Component({
@@ -80,7 +82,7 @@ export class SsoComponent implements OnInit, OnDestroy {
     { name: "Form POST", value: OpenIdConnectRedirectBehavior.FormPost },
   ];
 
-  private destory$ = new Subject<void>();
+  private destroy$ = new Subject<void>();
 
   showOpenIdCustomizations = false;
 
@@ -96,12 +98,6 @@ export class SsoComponent implements OnInit, OnDestroy {
   spMetadataUrl: string;
   spAcsUrl: string;
 
-  protected enabled = this.formBuilder.control(false);
-
-  protected ssoIdentifier = this.formBuilder.control("", {
-    validators: [Validators.maxLength(50), Validators.required],
-  });
-
   protected openIdForm = this.formBuilder.group<ControlsOf<SsoConfigView["openId"]>>(
     {
       authority: new FormControl("", Validators.required),
@@ -155,8 +151,22 @@ export class SsoComponent implements OnInit, OnDestroy {
     keyConnectorUrl: new FormControl(""),
     openId: this.openIdForm,
     saml: this.samlForm,
+    enabled: new FormControl(false),
+    ssoIdentifier: new FormControl("", {
+      validators: [Validators.maxLength(50), Validators.required],
+    }),
   });
 
+  get enabledCtrl() {
+    return this.ssoConfigForm?.controls?.enabled as FormControl;
+  }
+  get ssoIdentifierCtrl() {
+    return this.ssoConfigForm?.controls?.ssoIdentifier as FormControl;
+  }
+  get configTypeCtrl() {
+    return this.ssoConfigForm?.controls?.configType as FormControl;
+  }
+
   constructor(
     private formBuilder: FormBuilder,
     private route: ActivatedRoute,
@@ -168,9 +178,24 @@ export class SsoComponent implements OnInit, OnDestroy {
   ) {}
 
   async ngOnInit() {
+    this.enabledCtrl.valueChanges.pipe(takeUntil(this.destroy$)).subscribe((enabled) => {
+      if (enabled) {
+        this.ssoIdentifierCtrl.setValidators([Validators.maxLength(50), Validators.required]);
+        this.configTypeCtrl.setValidators([
+          ssoTypeValidator(this.i18nService.t("selectionIsRequired")),
+        ]);
+      } else {
+        this.ssoIdentifierCtrl.setValidators([]);
+        this.configTypeCtrl.setValidators([]);
+      }
+
+      this.ssoIdentifierCtrl.updateValueAndValidity();
+      this.configTypeCtrl.updateValueAndValidity();
+    });
+
     this.ssoConfigForm
       .get("configType")
-      .valueChanges.pipe(takeUntil(this.destory$))
+      .valueChanges.pipe(takeUntil(this.destroy$))
       .subscribe((newType: SsoType) => {
         if (newType === SsoType.OpenIdConnect) {
           this.openIdForm.enable();
@@ -186,7 +211,7 @@ export class SsoComponent implements OnInit, OnDestroy {
 
     this.samlForm
       .get("spSigningBehavior")
-      .valueChanges.pipe(takeUntil(this.destory$))
+      .valueChanges.pipe(takeUntil(this.destroy$))
       .subscribe(() => this.samlForm.get("idpX509PublicCert").updateValueAndValidity());
 
     this.route.params
@@ -195,14 +220,14 @@ export class SsoComponent implements OnInit, OnDestroy {
           this.organizationId = params.organizationId;
           await this.load();
         }),
-        takeUntil(this.destory$)
+        takeUntil(this.destroy$)
       )
       .subscribe();
   }
 
   ngOnDestroy(): void {
-    this.destory$.next();
-    this.destory$.complete();
+    this.destroy$.next();
+    this.destroy$.complete();
   }
 
   async load() {
@@ -220,7 +245,7 @@ export class SsoComponent implements OnInit, OnDestroy {
   }
 
   async submit() {
-    this.validateForm(this.ssoConfigForm);
+    this.updateFormValidationState(this.ssoConfigForm);
 
     if (this.ssoConfigForm.value.keyConnectorEnabled) {
       this.haveTestedKeyConnector = false;
@@ -231,10 +256,10 @@ export class SsoComponent implements OnInit, OnDestroy {
       this.readOutErrors();
       return;
     }
-
     const request = new OrganizationSsoRequest();
-    request.enabled = this.enabled.value;
-    request.identifier = this.ssoIdentifier.value;
+    request.enabled = this.enabledCtrl.value;
+    // Return null instead of empty string to avoid duplicate id errors in database
+    request.identifier = this.ssoIdentifierCtrl.value === "" ? null : this.ssoIdentifierCtrl.value;
     request.data = SsoConfigApi.fromView(this.ssoConfigForm.getRawValue());
 
     this.formPromise = this.organizationApiService.updateSso(this.organizationId, request);
@@ -301,14 +326,19 @@ export class SsoComponent implements OnInit, OnDestroy {
     return this.samlSigningAlgorithms.map((algorithm) => ({ name: algorithm, value: algorithm }));
   }
 
-  private validateForm(form: UntypedFormGroup) {
+  /**
+   * Shows any validation errors for the form by marking all controls as dirty and touched.
+   * If nested form groups are found, they are also updated.
+   * @param form - the form to show validation errors for
+   */
+  private updateFormValidationState(form: UntypedFormGroup) {
     Object.values(form.controls).forEach((control: AbstractControl) => {
       if (control.disabled) {
         return;
       }
 
       if (control instanceof UntypedFormGroup) {
-        this.validateForm(control);
+        this.updateFormValidationState(control);
       } else {
         control.markAsDirty();
         control.markAsTouched();
@@ -317,13 +347,9 @@ export class SsoComponent implements OnInit, OnDestroy {
     });
   }
 
-  private populateForm(ssoSettings: OrganizationSsoResponse) {
-    this.enabled.setValue(ssoSettings.enabled);
-    this.ssoIdentifier.setValue(ssoSettings.identifier);
-    if (ssoSettings.data != null) {
-      const ssoConfigView = new SsoConfigView(ssoSettings.data);
-      this.ssoConfigForm.patchValue(ssoConfigView);
-    }
+  private populateForm(orgSsoResponse: OrganizationSsoResponse) {
+    const ssoConfigView = new SsoConfigView(orgSsoResponse);
+    this.ssoConfigForm.patchValue(ssoConfigView);
   }
 
   private readOutErrors() {

bitwarden_license/bit-web/src/app/organizations/manage/domain-verification/domain-verification.component.html

@@ -30,9 +30,15 @@
         <ng-template body>
           <tr bitRow *ngFor="let orgDomain of orgDomains; index as i">
             <td bitCell>
-              <a bitLink href appStopClick linkType="primary" (click)="editDomain(orgDomain)">{{
-                orgDomain.domainName
-              }}</a>
+              <button
+                bitLink
+                type="button"
+                linkType="primary"
+                (click)="editDomain(orgDomain)"
+                appA11yTitle="{{ 'editDomain' | i18n }}"
+              >
+                {{ orgDomain.domainName }}
+              </button>
             </td>
             <td bitCell>
               <span *ngIf="!orgDomain?.verifiedDate" bitBadge badgeType="warning">{{

bitwarden_license/bit-web/src/app/organizations/organizations-routing.module.ts

@@ -8,7 +8,7 @@ import { OrganizationPermissionsGuard } from "@bitwarden/web-vault/app/organizat
 import { OrganizationLayoutComponent } from "@bitwarden/web-vault/app/organizations/layouts/organization-layout.component";
 import { SettingsComponent } from "@bitwarden/web-vault/app/organizations/settings/settings.component";
 
-import { SsoComponent } from "../auth/sso.component";
+import { SsoComponent } from "../auth/sso/sso.component";
 
 import { DomainVerificationComponent } from "./manage/domain-verification/domain-verification.component";
 import { ScimComponent } from "./manage/scim.component";

bitwarden_license/bit-web/src/app/organizations/organizations.module.ts

@@ -2,7 +2,7 @@ import { NgModule } from "@angular/core";
 
 import { SharedModule } from "@bitwarden/web-vault/app/shared/shared.module";
 
-import { SsoComponent } from "../auth/sso.component";
+import { SsoComponent } from "../auth/sso/sso.component";
 
 import { InputCheckboxComponent } from "./components/input-checkbox.component";
 import { DomainAddEditDialogComponent } from "./manage/domain-verification/domain-add-edit-dialog/domain-add-edit-dialog.component";

libs/common/src/auth/models/view/sso-config.view.ts

@@ -6,9 +6,12 @@ import {
   Saml2SigningBehavior,
   SsoType,
 } from "../../enums/sso";
-import { SsoConfigApi } from "../api/sso-config.api";
+import { OrganizationSsoResponse } from "../response/organization-sso.response";
 
 export class SsoConfigView extends View {
+  enabled: boolean;
+  ssoIdentifier: string;
+
   configType: SsoType;
 
   keyConnectorEnabled: boolean;
@@ -48,55 +51,63 @@ export class SsoConfigView extends View {
     idpWantAuthnRequestsSigned: boolean;
   };
 
-  constructor(api: SsoConfigApi) {
+  constructor(orgSsoResponse: OrganizationSsoResponse) {
     super();
-    if (api == null) {
+
+    if (orgSsoResponse == null) {
       return;
     }
 
-    this.configType = api.configType;
+    this.enabled = orgSsoResponse.enabled;
+    this.ssoIdentifier = orgSsoResponse.identifier;
 
-    this.keyConnectorEnabled = api.keyConnectorEnabled;
-    this.keyConnectorUrl = api.keyConnectorUrl;
+    if (orgSsoResponse.data == null) {
+      return;
+    }
+
+    this.configType = orgSsoResponse.data.configType;
+
+    this.keyConnectorEnabled = orgSsoResponse.data.keyConnectorEnabled;
+    this.keyConnectorUrl = orgSsoResponse.data.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,
+        authority: orgSsoResponse.data.authority,
+        clientId: orgSsoResponse.data.clientId,
+        clientSecret: orgSsoResponse.data.clientSecret,
+        metadataAddress: orgSsoResponse.data.metadataAddress,
+        redirectBehavior: orgSsoResponse.data.redirectBehavior,
+        getClaimsFromUserInfoEndpoint: orgSsoResponse.data.getClaimsFromUserInfoEndpoint,
+        additionalScopes: orgSsoResponse.data.additionalScopes,
+        additionalUserIdClaimTypes: orgSsoResponse.data.additionalUserIdClaimTypes,
+        additionalEmailClaimTypes: orgSsoResponse.data.additionalEmailClaimTypes,
+        additionalNameClaimTypes: orgSsoResponse.data.additionalNameClaimTypes,
+        acrValues: orgSsoResponse.data.acrValues,
+        expectedReturnAcrValue: orgSsoResponse.data.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,
+        spNameIdFormat: orgSsoResponse.data.spNameIdFormat,
+        spOutboundSigningAlgorithm: orgSsoResponse.data.spOutboundSigningAlgorithm,
+        spSigningBehavior: orgSsoResponse.data.spSigningBehavior,
+        spMinIncomingSigningAlgorithm: orgSsoResponse.data.spMinIncomingSigningAlgorithm,
+        spWantAssertionsSigned: orgSsoResponse.data.spWantAssertionsSigned,
+        spValidateCertificates: orgSsoResponse.data.spValidateCertificates,
 
-        idpEntityId: api.idpEntityId,
-        idpBindingType: api.idpBindingType,
-        idpSingleSignOnServiceUrl: api.idpSingleSignOnServiceUrl,
-        idpSingleLogoutServiceUrl: api.idpSingleLogoutServiceUrl,
-        idpX509PublicCert: api.idpX509PublicCert,
-        idpOutboundSigningAlgorithm: api.idpOutboundSigningAlgorithm,
-        idpAllowUnsolicitedAuthnResponse: api.idpAllowUnsolicitedAuthnResponse,
-        idpWantAuthnRequestsSigned: api.idpWantAuthnRequestsSigned,
+        idpEntityId: orgSsoResponse.data.idpEntityId,
+        idpBindingType: orgSsoResponse.data.idpBindingType,
+        idpSingleSignOnServiceUrl: orgSsoResponse.data.idpSingleSignOnServiceUrl,
+        idpSingleLogoutServiceUrl: orgSsoResponse.data.idpSingleLogoutServiceUrl,
+        idpX509PublicCert: orgSsoResponse.data.idpX509PublicCert,
+        idpOutboundSigningAlgorithm: orgSsoResponse.data.idpOutboundSigningAlgorithm,
+        idpAllowUnsolicitedAuthnResponse: orgSsoResponse.data.idpAllowUnsolicitedAuthnResponse,
+        idpWantAuthnRequestsSigned: orgSsoResponse.data.idpWantAuthnRequestsSigned,
 
         // Value is inverted in the view model (allow instead of disable)
         idpAllowOutboundLogoutRequests:
-          api.idpDisableOutboundLogoutRequests == null
+          orgSsoResponse.data.idpDisableOutboundLogoutRequests == null
             ? null
-            : !api.idpDisableOutboundLogoutRequests,
+            : !orgSsoResponse.data.idpDisableOutboundLogoutRequests,
       };
     }
   }