This repository was archived by the owner on Jun 17, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 138
Improve SSO Config validation #572
Merged
Merged
Changes from all commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
1d3de8e
Extract SsoConfig enums to own file
eliykat 69f41a9
Add changeClearErrorsDirective
eliykat 6954ba0
Export SsoUrls
eliykat ecf002d
Add ChangeStripSpaces directive
eliykat d8acd71
Move custom validators to jslib
eliykat 0256ea8
Fix typing
eliykat 994d4d3
Add a11y-invalid directive
eliykat 0ff7138
Add and implement dirtyValidator
eliykat e2fe543
Strip spaces on input event instead of change
eliykat f27a9a1
Fix linting
eliykat 6c4c5c7
Add None to SsoType enum
eliykat aee297e
Create ssoConfigView model and factory methods
eliykat 56fd31b
Add prebuilt dirty validators
eliykat 7c719c1
Do not export internal SsoUrls class
eliykat c2646f0
Merge commit '8b2dfc6cdcb8ff5b604364c2ea6d343473aee7cd' into fix/sso-…
eliykat 4e67874
Run prettier
eliykat 4b1850a
Merge commit '193434461dbd9c48fe5dcbad95693470aec422ac' into fix/sso-…
eliykat 45b5225
Remove old TODO
eliykat 24c0f66
Merge branch 'master' into fix/sso-validation
eliykat ef579ab
Merge branch 'master' into fix/sso-validation
eliykat d4e034e
Refactor dirtyRequired
eliykat 1b499d1
Add interface for select options
eliykat dba76df
Fix prettier
eliykat cc8678a
Fix syntax
eliykat 1e2fbaa
Update comment
eliykat 678e1c2
Rename interface
eliykat ea9cce7
Merge branch 'fix/sso-validation' of https://github.com/bitwarden/jsl…
eliykat 6f54045
Don't build SsoConfigData if null
eliykat 581635e
Fix prettier
eliykat 72779f6
Add null check in unsubscribe
eliykat da8c480
Fix selector
eliykat ad2e89e
Add jsdoc comments
eliykat 0501b4d
Merge branch 'fix/sso-validation' of https://github.com/bitwarden/jsl…
eliykat 23ef3e7
Fix linting
eliykat ae0843f
Run prettier
eliykat File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<HTMLInputElement>, 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(); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import { Directive, ElementRef, HostListener } from "@angular/core"; | ||
|
|
||
| @Directive({ | ||
| selector: "input[appInputStripSpaces]", | ||
| }) | ||
| export class InputStripSpacesDirective { | ||
| constructor(private el: ElementRef<HTMLInputElement>) {} | ||
|
|
||
| @HostListener("input") onInput() { | ||
| this.el.nativeElement.value = this.el.nativeElement.value.replace(/ /g, ""); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| export interface SelectOptions { | ||
| name: string; | ||
| value: any; | ||
| disabled?: boolean; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
| }; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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, | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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, | ||
| }; | ||
| } | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work? We send defaults from the server which we should receive on the client to show the correct defaults.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I didn't realise that. I've provided the defaults client-side in the related
webPR (see here). To me it makes more sense to do this client-side, because they are form defaults and the form belongs to the client. I also think it's required for the new design of the form (now that it only saves the selected configuration). I didn't notice any unexpected behaviour during my testing.Note that the read-only fields (
callbackPath,entityId,metadataUrl, etc) are still populated from the data provided by the server (for both configurations), they're outside thessoConfigobject.What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although if we do that then I guess we should remove the defaults server-side and make sure that won't affect anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that. Can you open a PR on the server with the removal of the defaults?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hinton I came back to this and have done the following: