From 420e45f2301f9cff19a8aeb9cf01f54d93fabc9e Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Mon, 15 Jan 2024 12:08:10 +0100 Subject: [PATCH 01/20] feat(permissions): filter fields in forms based on read/write permissions - filter fields in forms with no read permission - disable fields in edit mode with read-only permissions --- src/app/app.component.spec.ts | 4 +- .../entity-form/entity-form.service.ts | 2 +- .../entity-details/form/form.component.html | 2 +- .../form/form.component.spec.ts | 5 ++ .../entity-details/form/form.component.ts | 48 +++++++++++++++++-- .../permissions/ability/ability.service.ts | 36 +++++++++++++- .../demo-permission-generator.service.ts | 5 +- .../disable-entity-operation.directive.ts | 4 +- .../disabled-wrapper.component.ts | 5 ++ 9 files changed, 101 insertions(+), 10 deletions(-) diff --git a/src/app/app.component.spec.ts b/src/app/app.component.spec.ts index 950c3255ea..11b9035cba 100644 --- a/src/app/app.component.spec.ts +++ b/src/app/app.component.spec.ts @@ -41,11 +41,11 @@ describe("AppComponent", () => { fixture.detectChanges(); })); - afterEach(() => { + afterEach(waitForAsync(() => { environment.demo_mode = false; jasmine.DEFAULT_TIMEOUT_INTERVAL = intervalBefore; return TestBed.inject(Database).destroy(); - }); + })); it("should be created", () => { expect(component).toBeTruthy(); diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index 9a8f536b15..ca820f7b28 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -233,7 +233,7 @@ export class EntityFormService { } private checkFormValidity(form: EntityForm) { - // errors regarding invalid fields wont be displayed unless marked as touched + // errors regarding invalid fields won't be displayed unless marked as touched form.markAllAsTouched(); if (form.invalid) { throw new InvalidFormFieldError(); diff --git a/src/app/core/entity-details/form/form.component.html b/src/app/core/entity-details/form/form.component.html index cd96cab994..be4bc8743b 100644 --- a/src/app/core/entity-details/form/form.component.html +++ b/src/app/core/entity-details/form/form.component.html @@ -37,6 +37,6 @@ diff --git a/src/app/core/entity-details/form/form.component.spec.ts b/src/app/core/entity-details/form/form.component.spec.ts index ecdf23af5d..d6aa6749df 100644 --- a/src/app/core/entity-details/form/form.component.spec.ts +++ b/src/app/core/entity-details/form/form.component.spec.ts @@ -7,16 +7,21 @@ import { MockedTestingModule } from "../../../utils/mocked-testing.module"; import { ConfirmationDialogService } from "../../common-components/confirmation-dialog/confirmation-dialog.service"; import { AlertService } from "../../alerts/alert.service"; import { EntityFormService } from "../../common-components/entity-form/entity-form.service"; +import { AbilityService } from "../../permissions/ability/ability.service"; describe("FormComponent", () => { let component: FormComponent; let fixture: ComponentFixture>; + let abilityService: AbilityService; + beforeEach(waitForAsync(() => { TestBed.configureTestingModule({ imports: [FormComponent, MockedTestingModule.withState()], providers: [{ provide: ConfirmationDialogService, useValue: null }], }).compileComponents(); + abilityService = TestBed.inject(AbilityService); + abilityService.initializeRules(); })); beforeEach(() => { diff --git a/src/app/core/entity-details/form/form.component.ts b/src/app/core/entity-details/form/form.component.ts index bc6e7d2da3..51fc90ab79 100644 --- a/src/app/core/entity-details/form/form.component.ts +++ b/src/app/core/entity-details/form/form.component.ts @@ -1,4 +1,4 @@ -import { Component, Input, OnInit } from "@angular/core"; +import { Component, Input, OnDestroy, OnInit } from "@angular/core"; import { Entity } from "../../entity/model/entity"; import { getParentUrl } from "../../../utils/utils"; import { Router } from "@angular/router"; @@ -14,6 +14,8 @@ import { MatButtonModule } from "@angular/material/button"; import { EntityFormComponent } from "../../common-components/entity-form/entity-form/entity-form.component"; import { DisableEntityOperationDirective } from "../../permissions/permission-directive/disable-entity-operation.directive"; import { FieldGroup } from "./field-group"; +import { Subject, takeUntil } from "rxjs"; +import { AbilityService } from "../../permissions/ability/ability.service"; /** * A simple wrapper function of the EntityFormComponent which can be used as a dynamic component @@ -32,28 +34,49 @@ import { FieldGroup } from "./field-group"; ], standalone: true, }) -export class FormComponent implements FormConfig, OnInit { +export class FormComponent + implements FormConfig, OnInit, OnDestroy +{ @Input() entity: E; @Input() creatingNew = false; @Input() fieldGroups: FieldGroup[]; + filteredFieldGroups: FieldGroup[]; + form: EntityForm; + private _destroyed = new Subject(); + constructor( private router: Router, private location: Location, private entityFormService: EntityFormService, private alertService: AlertService, + private abilityService: AbilityService, ) {} + ngOnDestroy(): void { + this._destroyed.next(); + this._destroyed.complete(); + } + ngOnInit() { + this.filterFieldGroupsByPermissions(); + this.form = this.entityFormService.createFormGroup( - [].concat(...this.fieldGroups.map((group) => group.fields)), + [].concat(...this.filteredFieldGroups.map((group) => group.fields)), this.entity, ); + + this.form.statusChanges.pipe(takeUntil(this._destroyed)).subscribe((_) => { + this.disableReadOnlyFormControls(); + }); + if (!this.creatingNew) { this.form.disable(); + } else { + this.disableReadOnlyFormControls(); } } @@ -82,6 +105,25 @@ export class FormComponent implements FormConfig, OnInit { this.entityFormService.resetForm(this.form, this.entity); this.form.disable(); } + + private filterFieldGroupsByPermissions() { + this.filteredFieldGroups = this.fieldGroups + .map((group) => { + group.fields = group.fields.filter((field) => + this.abilityService.hasReadPermission(this.entity, field), + ); + return group; + }) + .filter((group) => group.fields.length !== 0); + } + + private disableReadOnlyFormControls() { + Object.keys(this.form.controls).forEach((fieldId) => { + if (!this.abilityService.hasEditPermission(this.entity, fieldId)) { + this.form.get(fieldId).disable({ onlySelf: true, emitEvent: false }); + } + }); + } } /** diff --git a/src/app/core/permissions/ability/ability.service.ts b/src/app/core/permissions/ability/ability.service.ts index f14e5d40e9..af04c94a5e 100644 --- a/src/app/core/permissions/ability/ability.service.ts +++ b/src/app/core/permissions/ability/ability.service.ts @@ -1,5 +1,5 @@ import { Injectable } from "@angular/core"; -import { DatabaseRule, DatabaseRules } from "../permission-types"; +import { DatabaseRule, DatabaseRules, EntityAction } from "../permission-types"; import { EntityMapperService } from "../../entity/entity-mapper/entity-mapper.service"; import { PermissionEnforcerService } from "../permission-enforcer/permission-enforcer.service"; import { EntityAbility } from "./entity-ability"; @@ -8,6 +8,8 @@ import { LoggingService } from "../../logging/logging.service"; import { get } from "lodash-es"; import { LatestEntityLoader } from "../../entity/latest-entity-loader"; import { SessionInfo, SessionSubject } from "../../session/auth/session-info"; +import { FormFieldConfig } from "../../common-components/entity-form/entity-form/FormConfig"; +import { Entity } from "../../entity/model/entity"; /** * This service sets up the `EntityAbility` injectable with the JSON defined rules for the currently logged in user. @@ -17,6 +19,16 @@ import { SessionInfo, SessionSubject } from "../../session/auth/session-info"; */ @Injectable() export class AbilityService extends LatestEntityLoader> { + readonly READ_PERMISSION_GROUP: EntityAction[] = [ + "read", + "create", + "update", + "manage", + "delete", + ]; + + readonly WRITE_PERMISSION_GROUP: EntityAction[] = ["update", "manage"]; + constructor( private ability: EntityAbility, private sessionInfo: SessionSubject, @@ -27,6 +39,14 @@ export class AbilityService extends LatestEntityLoader> { super(Config, Config.PERMISSION_KEY, entityMapper, logger); } + hasReadPermission(entity: Entity, field: string | FormFieldConfig): boolean { + return this._hasFieldPermission(entity, field, this.READ_PERMISSION_GROUP); + } + + hasEditPermission(entity: Entity, field: string | FormFieldConfig): boolean { + return this._hasFieldPermission(entity, field, this.WRITE_PERMISSION_GROUP); + } + async initializeRules() { const initialPermissions = await super.startLoading(); if (initialPermissions) { @@ -93,4 +113,18 @@ export class AbilityService extends LatestEntityLoader> { return value; }); } + + private _hasFieldPermission( + entity: Entity, + field: string | FormFieldConfig, + permissionGroup: EntityAction[], + ) { + let fieldId: string = typeof field === "string" ? field : field.id; + for (let i = 0; i < permissionGroup.length; i++) { + if (this.ability.can(permissionGroup[i], entity, fieldId)) { + return true; + } + } + return false; + } } diff --git a/src/app/core/permissions/demo-permission-generator.service.ts b/src/app/core/permissions/demo-permission-generator.service.ts index b0a6ec4f73..9e340a5c2a 100644 --- a/src/app/core/permissions/demo-permission-generator.service.ts +++ b/src/app/core/permissions/demo-permission-generator.service.ts @@ -20,7 +20,10 @@ export class DemoPermissionGeneratorService extends DemoDataGenerator< // This can be changed to experiment with different permission setups locally. // For the general demo mode everything is allowed const rules: DatabaseRules = { - user_app: [{ subject: "all", action: "manage" }], + user_app: [ + { subject: "Child", action: "read" }, + { subject: "Child", action: "manage", fields: ["name", "dateOfBirth"] }, + ], admin_app: [{ subject: "all", action: "manage" }], }; return [new Config(Config.PERMISSION_KEY, rules)]; diff --git a/src/app/core/permissions/permission-directive/disable-entity-operation.directive.ts b/src/app/core/permissions/permission-directive/disable-entity-operation.directive.ts index 2548287941..458831a0be 100644 --- a/src/app/core/permissions/permission-directive/disable-entity-operation.directive.ts +++ b/src/app/core/permissions/permission-directive/disable-entity-operation.directive.ts @@ -26,12 +26,13 @@ export class DisableEntityOperationDirective { /** * These arguments are required to check whether the user has permissions to perform the operation. - * The operation property defines to what kind of operation a element belongs, e.g. OperationType.CREATE + * The operation property defines to what kind of operation an element belongs, e.g. OperationType.CREATE * The entity property defines for which kind of entity the operation will be performed, e.g. Child */ @Input("appDisabledEntityOperation") arguments: { operation: EntityAction; entity: EntitySubject; + field?: string; }; private wrapperComponent: ComponentRef; @@ -76,6 +77,7 @@ export class DisableEntityOperationDirective this.wrapperComponent.instance.elementDisabled = this.ability.cannot( this.arguments.operation, this.arguments.entity, + this.arguments.field, ); this.wrapperComponent.instance.ngAfterViewInit(); } diff --git a/src/app/core/permissions/permission-directive/disabled-wrapper.component.ts b/src/app/core/permissions/permission-directive/disabled-wrapper.component.ts index 359cacf3dc..583b47dec1 100644 --- a/src/app/core/permissions/permission-directive/disabled-wrapper.component.ts +++ b/src/app/core/permissions/permission-directive/disabled-wrapper.component.ts @@ -54,6 +54,11 @@ export class DisabledWrapperComponent implements AfterViewInit { if (this.wrapper) { const buttonElement = this.wrapper.nativeElement.getElementsByTagName("button")[0]; + + if (!buttonElement) { + return; + } + if (this.elementDisabled) { this.renderer.addClass(buttonElement, "mat-button-disabled"); this.renderer.setAttribute(buttonElement, "disabled", "true"); From 0e627bc53e020edeeb3b8542bef0e19f99e0111c Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Mon, 15 Jan 2024 12:26:01 +0100 Subject: [PATCH 02/20] fix: reset demo-permission-generator --- .../core/permissions/demo-permission-generator.service.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/app/core/permissions/demo-permission-generator.service.ts b/src/app/core/permissions/demo-permission-generator.service.ts index 9e340a5c2a..b0a6ec4f73 100644 --- a/src/app/core/permissions/demo-permission-generator.service.ts +++ b/src/app/core/permissions/demo-permission-generator.service.ts @@ -20,10 +20,7 @@ export class DemoPermissionGeneratorService extends DemoDataGenerator< // This can be changed to experiment with different permission setups locally. // For the general demo mode everything is allowed const rules: DatabaseRules = { - user_app: [ - { subject: "Child", action: "read" }, - { subject: "Child", action: "manage", fields: ["name", "dateOfBirth"] }, - ], + user_app: [{ subject: "all", action: "manage" }], admin_app: [{ subject: "all", action: "manage" }], }; return [new Config(Config.PERMISSION_KEY, rules)]; From ab8a9d64e0a708060de84f7ea4f597f490acb1e3 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Mon, 15 Jan 2024 12:45:57 +0100 Subject: [PATCH 03/20] refactor: disable-wrapper component for better Cognitive Complexity score --- .../disabled-wrapper.component.ts | 36 ++++++++++++------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/src/app/core/permissions/permission-directive/disabled-wrapper.component.ts b/src/app/core/permissions/permission-directive/disabled-wrapper.component.ts index 583b47dec1..b4e2599ee3 100644 --- a/src/app/core/permissions/permission-directive/disabled-wrapper.component.ts +++ b/src/app/core/permissions/permission-directive/disabled-wrapper.component.ts @@ -51,21 +51,31 @@ export class DisabledWrapperComponent implements AfterViewInit { constructor(private renderer: Renderer2) {} ngAfterViewInit() { - if (this.wrapper) { - const buttonElement = - this.wrapper.nativeElement.getElementsByTagName("button")[0]; + if (!this.wrapper) { + return; + } + + const buttonElement = + this.wrapper.nativeElement.getElementsByTagName("button")[0]; - if (!buttonElement) { - return; - } + if (!buttonElement) { + return; + } - if (this.elementDisabled) { - this.renderer.addClass(buttonElement, "mat-button-disabled"); - this.renderer.setAttribute(buttonElement, "disabled", "true"); - } else { - this.renderer.removeAttribute(buttonElement, "disabled"); - this.renderer.removeClass(buttonElement, "mat-button-disabled"); - } + if (this.elementDisabled) { + this.disable(buttonElement); + } else { + this.enable(buttonElement); } } + + private enable(buttonElement: HTMLButtonElement) { + this.renderer.removeAttribute(buttonElement, "disabled"); + this.renderer.removeClass(buttonElement, "mat-button-disabled"); + } + + private disable(buttonElement: HTMLButtonElement) { + this.renderer.addClass(buttonElement, "mat-button-disabled"); + this.renderer.setAttribute(buttonElement, "disabled", "true"); + } } From 020d3bba214ec781e3ca3b689f37b9d3c8f56c69 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Mon, 15 Jan 2024 13:51:08 +0100 Subject: [PATCH 04/20] fix: use correct import path in ability.service.ts --- src/app/core/permissions/ability/ability.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/core/permissions/ability/ability.service.ts b/src/app/core/permissions/ability/ability.service.ts index af04c94a5e..b59a726871 100644 --- a/src/app/core/permissions/ability/ability.service.ts +++ b/src/app/core/permissions/ability/ability.service.ts @@ -8,8 +8,8 @@ import { LoggingService } from "../../logging/logging.service"; import { get } from "lodash-es"; import { LatestEntityLoader } from "../../entity/latest-entity-loader"; import { SessionInfo, SessionSubject } from "../../session/auth/session-info"; -import { FormFieldConfig } from "../../common-components/entity-form/entity-form/FormConfig"; import { Entity } from "../../entity/model/entity"; +import { FormFieldConfig } from "../../common-components/entity-form/FormConfig"; /** * This service sets up the `EntityAbility` injectable with the JSON defined rules for the currently logged in user. From e5301eb4eccf394ea9579e19f09483ab2106bd59 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Mon, 15 Jan 2024 16:57:50 +0100 Subject: [PATCH 05/20] refactor: move filter logic to entity-form component/service --- .../entity-form/entity-form.service.ts | 30 ++++++++++++ .../entity-form/entity-form.component.ts | 26 +++++++++- .../entity-select.component.spec.ts | 4 ++ .../entity-details/form/form.component.html | 2 +- .../form/form.component.spec.ts | 8 ++-- .../entity-details/form/form.component.ts | 47 ++----------------- .../entity-list/entity-list.component.spec.ts | 4 ++ .../permissions/ability/ability.service.ts | 36 +------------- 8 files changed, 72 insertions(+), 85 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index b801757ed1..cc6b9d2d8f 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -18,6 +18,7 @@ import { } from "../../entity/schema/entity-schema-field"; import { isArrayDataType } from "../../basic-datatypes/datatype-utils"; import { CurrentUserSubject } from "../../session/current-user-subject"; +import { FieldGroup } from "../../entity-details/form/field-group"; /** * These are utility types that allow to define the type of `FormGroup` the way it is returned by `EntityFormService.create` @@ -78,6 +79,35 @@ export class EntityFormService { } } + filterFieldGroupsByPermissions( + fieldGroups: FieldGroup[], + entity: T, + ): FieldGroup[] { + return fieldGroups + .map((group) => { + group.fields = group.fields.filter((field) => + this.ability.can( + "read", + entity, + typeof field === "string" ? field : field.id, + ), + ); + return group; + }) + .filter((group) => group.fields.length !== 0); + } + + disableReadOnlyFormControls( + form: EntityForm, + entity: T, + ) { + Object.keys(form.controls).forEach((fieldId) => { + if (!this.ability.can("update", entity, fieldId)) { + form.get(fieldId).disable({ onlySelf: true, emitEvent: false }); + } + }); + } + private addSchemaToFormField( formField: FormFieldConfig, propertySchema: EntitySchemaField, diff --git a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts index 1d392e4447..1bb928087c 100644 --- a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts +++ b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts @@ -6,7 +6,7 @@ import { ViewEncapsulation, } from "@angular/core"; import { Entity } from "../../../entity/model/entity"; -import { EntityForm } from "../entity-form.service"; +import { EntityForm, EntityFormService } from "../entity-form.service"; import { EntityMapperService } from "../../../entity/entity-mapper/entity-mapper.service"; import { filter } from "rxjs/operators"; import { ConfirmationDialogService } from "../../confirmation-dialog/confirmation-dialog.service"; @@ -60,9 +60,26 @@ export class EntityFormComponent constructor( private entityMapper: EntityMapperService, private confirmationDialog: ConfirmationDialogService, + private entityFormService: EntityFormService, ) {} ngOnChanges(changes: SimpleChanges) { + if (this.fieldGroups) { + this.fieldGroups = this.entityFormService.filterFieldGroupsByPermissions( + this.fieldGroups, + this.entit, + ); + } + + if (this.form) { + this.form.statusChanges.pipe(untilDestroyed(this)).subscribe((_) => { + this.entityFormService.disableReadOnlyFormControls( + this.form, + this.entit, + ); + }); + } + if (changes.entity && this.entity) { this.changesSubscription?.unsubscribe(); this.changesSubscription = this.entityMapper @@ -78,6 +95,13 @@ export class EntityFormComponent this.initialFormValues = this.form.getRawValue(); this.disableForLockedEntity(); } + + if (this.form && this.entity) { + this.entityFormService.disableReadOnlyFormControls( + this.form, + this.entity, + ); + } } private async applyChanges(externallyUpdatedEntity: T) { diff --git a/src/app/core/common-components/entity-select/entity-select.component.spec.ts b/src/app/core/common-components/entity-select/entity-select.component.spec.ts index b474f8034c..4b7157dc56 100644 --- a/src/app/core/common-components/entity-select/entity-select.component.spec.ts +++ b/src/app/core/common-components/entity-select/entity-select.component.spec.ts @@ -12,10 +12,12 @@ import { Child } from "../../../child-dev-project/children/model/child"; import { School } from "../../../child-dev-project/schools/model/school"; import { MockedTestingModule } from "../../../utils/mocked-testing.module"; import { LoginState } from "../../session/session-states/login-state.enum"; +import { EntityAbility } from "../../permissions/ability/entity-ability"; describe("EntitySelectComponent", () => { let component: EntitySelectComponent; let fixture: ComponentFixture>; + let entityAbility: EntityAbility; const testUsers: Entity[] = ["Abc", "Bcd", "Abd", "Aba"].map((s) => { const user = new User(); @@ -36,6 +38,8 @@ describe("EntitySelectComponent", () => { ]), ], }).compileComponents(); + entityAbility = TestBed.inject(EntityAbility); + spyOn(entityAbility, "can").and.returnValue(true); })); beforeEach(() => { diff --git a/src/app/core/entity-details/form/form.component.html b/src/app/core/entity-details/form/form.component.html index be4bc8743b..cd96cab994 100644 --- a/src/app/core/entity-details/form/form.component.html +++ b/src/app/core/entity-details/form/form.component.html @@ -37,6 +37,6 @@ diff --git a/src/app/core/entity-details/form/form.component.spec.ts b/src/app/core/entity-details/form/form.component.spec.ts index d6aa6749df..eea9c0bb63 100644 --- a/src/app/core/entity-details/form/form.component.spec.ts +++ b/src/app/core/entity-details/form/form.component.spec.ts @@ -7,21 +7,21 @@ import { MockedTestingModule } from "../../../utils/mocked-testing.module"; import { ConfirmationDialogService } from "../../common-components/confirmation-dialog/confirmation-dialog.service"; import { AlertService } from "../../alerts/alert.service"; import { EntityFormService } from "../../common-components/entity-form/entity-form.service"; -import { AbilityService } from "../../permissions/ability/ability.service"; +import { EntityAbility } from "../../permissions/ability/entity-ability"; describe("FormComponent", () => { let component: FormComponent; let fixture: ComponentFixture>; - let abilityService: AbilityService; + let entityAbility: EntityAbility; beforeEach(waitForAsync(() => { TestBed.configureTestingModule({ imports: [FormComponent, MockedTestingModule.withState()], providers: [{ provide: ConfirmationDialogService, useValue: null }], }).compileComponents(); - abilityService = TestBed.inject(AbilityService); - abilityService.initializeRules(); + entityAbility = TestBed.inject(EntityAbility); + spyOn(entityAbility, "can").and.returnValue(true); })); beforeEach(() => { diff --git a/src/app/core/entity-details/form/form.component.ts b/src/app/core/entity-details/form/form.component.ts index 51fc90ab79..1071f27b70 100644 --- a/src/app/core/entity-details/form/form.component.ts +++ b/src/app/core/entity-details/form/form.component.ts @@ -1,4 +1,4 @@ -import { Component, Input, OnDestroy, OnInit } from "@angular/core"; +import { Component, Input, OnInit } from "@angular/core"; import { Entity } from "../../entity/model/entity"; import { getParentUrl } from "../../../utils/utils"; import { Router } from "@angular/router"; @@ -14,8 +14,6 @@ import { MatButtonModule } from "@angular/material/button"; import { EntityFormComponent } from "../../common-components/entity-form/entity-form/entity-form.component"; import { DisableEntityOperationDirective } from "../../permissions/permission-directive/disable-entity-operation.directive"; import { FieldGroup } from "./field-group"; -import { Subject, takeUntil } from "rxjs"; -import { AbilityService } from "../../permissions/ability/ability.service"; /** * A simple wrapper function of the EntityFormComponent which can be used as a dynamic component @@ -34,49 +32,29 @@ import { AbilityService } from "../../permissions/ability/ability.service"; ], standalone: true, }) -export class FormComponent - implements FormConfig, OnInit, OnDestroy -{ +export class FormComponent implements FormConfig, OnInit { @Input() entity: E; @Input() creatingNew = false; @Input() fieldGroups: FieldGroup[]; - filteredFieldGroups: FieldGroup[]; - form: EntityForm; - private _destroyed = new Subject(); - constructor( private router: Router, private location: Location, private entityFormService: EntityFormService, private alertService: AlertService, - private abilityService: AbilityService, ) {} - ngOnDestroy(): void { - this._destroyed.next(); - this._destroyed.complete(); - } - ngOnInit() { - this.filterFieldGroupsByPermissions(); - this.form = this.entityFormService.createFormGroup( - [].concat(...this.filteredFieldGroups.map((group) => group.fields)), + [].concat(...this.fieldGroups.map((group) => group.fields)), this.entity, ); - this.form.statusChanges.pipe(takeUntil(this._destroyed)).subscribe((_) => { - this.disableReadOnlyFormControls(); - }); - if (!this.creatingNew) { this.form.disable(); - } else { - this.disableReadOnlyFormControls(); } } @@ -105,25 +83,6 @@ export class FormComponent this.entityFormService.resetForm(this.form, this.entity); this.form.disable(); } - - private filterFieldGroupsByPermissions() { - this.filteredFieldGroups = this.fieldGroups - .map((group) => { - group.fields = group.fields.filter((field) => - this.abilityService.hasReadPermission(this.entity, field), - ); - return group; - }) - .filter((group) => group.fields.length !== 0); - } - - private disableReadOnlyFormControls() { - Object.keys(this.form.controls).forEach((fieldId) => { - if (!this.abilityService.hasEditPermission(this.entity, fieldId)) { - this.form.get(fieldId).disable({ onlySelf: true, emitEvent: false }); - } - }); - } } /** diff --git a/src/app/core/entity-list/entity-list/entity-list.component.spec.ts b/src/app/core/entity-list/entity-list/entity-list.component.spec.ts index 0c95b72d70..6669807ec2 100644 --- a/src/app/core/entity-list/entity-list/entity-list.component.spec.ts +++ b/src/app/core/entity-list/entity-list/entity-list.component.spec.ts @@ -21,11 +21,13 @@ import { TestbedHarnessEnvironment } from "@angular/cdk/testing/testbed"; import { MatTabGroupHarness } from "@angular/material/tabs/testing"; import { FormDialogService } from "../../form-dialog/form-dialog.service"; import { UpdatedEntity } from "../../entity/model/entity-update"; +import { EntityAbility } from "../../permissions/ability/entity-ability"; describe("EntityListComponent", () => { let component: EntityListComponent; let fixture: ComponentFixture>; let loader: HarnessLoader; + let entityAbility: EntityAbility; const testConfig: EntityListConfig = { title: "Children List", @@ -95,6 +97,8 @@ describe("EntityListComponent", () => { { provide: FormDialogService, useValue: null }, ], }).compileComponents(); + entityAbility = TestBed.inject(EntityAbility); + spyOn(entityAbility, "can").and.returnValue(true); })); it("should create", () => { diff --git a/src/app/core/permissions/ability/ability.service.ts b/src/app/core/permissions/ability/ability.service.ts index b59a726871..f14e5d40e9 100644 --- a/src/app/core/permissions/ability/ability.service.ts +++ b/src/app/core/permissions/ability/ability.service.ts @@ -1,5 +1,5 @@ import { Injectable } from "@angular/core"; -import { DatabaseRule, DatabaseRules, EntityAction } from "../permission-types"; +import { DatabaseRule, DatabaseRules } from "../permission-types"; import { EntityMapperService } from "../../entity/entity-mapper/entity-mapper.service"; import { PermissionEnforcerService } from "../permission-enforcer/permission-enforcer.service"; import { EntityAbility } from "./entity-ability"; @@ -8,8 +8,6 @@ import { LoggingService } from "../../logging/logging.service"; import { get } from "lodash-es"; import { LatestEntityLoader } from "../../entity/latest-entity-loader"; import { SessionInfo, SessionSubject } from "../../session/auth/session-info"; -import { Entity } from "../../entity/model/entity"; -import { FormFieldConfig } from "../../common-components/entity-form/FormConfig"; /** * This service sets up the `EntityAbility` injectable with the JSON defined rules for the currently logged in user. @@ -19,16 +17,6 @@ import { FormFieldConfig } from "../../common-components/entity-form/FormConfig" */ @Injectable() export class AbilityService extends LatestEntityLoader> { - readonly READ_PERMISSION_GROUP: EntityAction[] = [ - "read", - "create", - "update", - "manage", - "delete", - ]; - - readonly WRITE_PERMISSION_GROUP: EntityAction[] = ["update", "manage"]; - constructor( private ability: EntityAbility, private sessionInfo: SessionSubject, @@ -39,14 +27,6 @@ export class AbilityService extends LatestEntityLoader> { super(Config, Config.PERMISSION_KEY, entityMapper, logger); } - hasReadPermission(entity: Entity, field: string | FormFieldConfig): boolean { - return this._hasFieldPermission(entity, field, this.READ_PERMISSION_GROUP); - } - - hasEditPermission(entity: Entity, field: string | FormFieldConfig): boolean { - return this._hasFieldPermission(entity, field, this.WRITE_PERMISSION_GROUP); - } - async initializeRules() { const initialPermissions = await super.startLoading(); if (initialPermissions) { @@ -113,18 +93,4 @@ export class AbilityService extends LatestEntityLoader> { return value; }); } - - private _hasFieldPermission( - entity: Entity, - field: string | FormFieldConfig, - permissionGroup: EntityAction[], - ) { - let fieldId: string = typeof field === "string" ? field : field.id; - for (let i = 0; i < permissionGroup.length; i++) { - if (this.ability.can(permissionGroup[i], entity, fieldId)) { - return true; - } - } - return false; - } } From 38f8c812174b54f62c99d4fd6ca23249b35ef4e2 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Mon, 15 Jan 2024 17:01:18 +0100 Subject: [PATCH 06/20] fix: typo --- .../entity-form/entity-form/entity-form.component.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts index 1bb928087c..0ece89ff8d 100644 --- a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts +++ b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts @@ -67,7 +67,7 @@ export class EntityFormComponent if (this.fieldGroups) { this.fieldGroups = this.entityFormService.filterFieldGroupsByPermissions( this.fieldGroups, - this.entit, + this.entity, ); } @@ -75,7 +75,7 @@ export class EntityFormComponent this.form.statusChanges.pipe(untilDestroyed(this)).subscribe((_) => { this.entityFormService.disableReadOnlyFormControls( this.form, - this.entit, + this.entity, ); }); } From 41d779c505ce57099fe7f0dec92f8e9f4e766464 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Mon, 15 Jan 2024 17:33:46 +0100 Subject: [PATCH 07/20] test: improve test coverage --- .../entity-form/entity-form.service.spec.ts | 41 +++++++++++++++++++ .../entity-form/entity-form.service.ts | 2 +- .../entity-form/entity-form.component.ts | 34 ++++++++------- 3 files changed, 61 insertions(+), 16 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.spec.ts b/src/app/core/common-components/entity-form/entity-form.service.spec.ts index 777745b9b0..53092a1e25 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.spec.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.spec.ts @@ -26,6 +26,7 @@ import { DatabaseField } from "../../entity/database-field.decorator"; import { EntitySchemaService } from "../../entity/schema/entity-schema.service"; import { FormFieldConfig } from "./FormConfig"; import { TEST_USER } from "../../user/demo-user-generator.service"; +import { FieldGroup } from "../../entity-details/form/field-group"; describe("EntityFormService", () => { let service: EntityFormService; @@ -54,6 +55,46 @@ describe("EntityFormService", () => { expect(entity.getId()).not.toBe("newId"); }); + it("should remove fields without read permissions", async () => { + const entity = new Entity(); + const fieldGroup: FieldGroup[] = [ + { fields: ["foo", "bar"] }, + { fields: ["name"] }, + { fields: ["birthday"] }, + ]; + + TestBed.inject(EntityAbility).update([ + { subject: "Entity", action: "read", fields: ["foo", "name"] }, + ]); + + const filteredFieldGroup = service.filterFieldGroupsByPermissions( + fieldGroup, + entity, + ); + + expect(filteredFieldGroup.length).toBe(2); + expect(filteredFieldGroup[0].fields.length).toBe(1); + expect(filteredFieldGroup[1].fields.length).toBe(1); + }); + + it("should remove controls with read-only permissions from form", async () => { + const entity = new Entity(); + const formGroup = new UntypedFormGroup({ + name: new UntypedFormControl("name"), + foo: new UntypedFormControl("foo"), + bar: new UntypedFormControl("bar"), + }); + TestBed.inject(EntityAbility).update([ + { subject: "Entity", action: "update", fields: ["foo"] }, + ]); + + service.disableReadOnlyFormControls(formGroup, entity); + + expect(formGroup.get("name").disabled).toBeTruthy(); + expect(formGroup.get("foo").disabled).toBeFalse(); + expect(formGroup.get("bar").disabled).toBeTruthy(); + }); + it("should update entity if saving is successful", async () => { const entity = new Entity("initialId"); const formGroup = new UntypedFormGroup({ diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index cc6b9d2d8f..317814df36 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -94,7 +94,7 @@ export class EntityFormService { ); return group; }) - .filter((group) => group.fields.length !== 0); + .filter((group) => group.fields.length > 0); } disableReadOnlyFormControls( diff --git a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts index 0ece89ff8d..7c2f96186c 100644 --- a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts +++ b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts @@ -64,21 +64,7 @@ export class EntityFormComponent ) {} ngOnChanges(changes: SimpleChanges) { - if (this.fieldGroups) { - this.fieldGroups = this.entityFormService.filterFieldGroupsByPermissions( - this.fieldGroups, - this.entity, - ); - } - - if (this.form) { - this.form.statusChanges.pipe(untilDestroyed(this)).subscribe((_) => { - this.entityFormService.disableReadOnlyFormControls( - this.form, - this.entity, - ); - }); - } + this.applyFormFieldPermissions(); if (changes.entity && this.entity) { this.changesSubscription?.unsubscribe(); @@ -104,6 +90,24 @@ export class EntityFormComponent } } + private applyFormFieldPermissions() { + if (this.fieldGroups) { + this.fieldGroups = this.entityFormService.filterFieldGroupsByPermissions( + this.fieldGroups, + this.entity, + ); + } + + if (this.form) { + this.form.statusChanges.pipe(untilDestroyed(this)).subscribe((_) => { + this.entityFormService.disableReadOnlyFormControls( + this.form, + this.entity, + ); + }); + } + } + private async applyChanges(externallyUpdatedEntity: T) { if (this.formIsUpToDate(externallyUpdatedEntity)) { Object.assign(this.entity, externallyUpdatedEntity); From 5248fe0727a29650338b24df376b060e364fd305 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Tue, 16 Jan 2024 09:03:11 +0100 Subject: [PATCH 08/20] fix: test syntax Co-authored-by: Simon --- .../entity-form/entity-form.service.spec.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.spec.ts b/src/app/core/common-components/entity-form/entity-form.service.spec.ts index 53092a1e25..79bc34cde4 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.spec.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.spec.ts @@ -72,9 +72,10 @@ describe("EntityFormService", () => { entity, ); - expect(filteredFieldGroup.length).toBe(2); - expect(filteredFieldGroup[0].fields.length).toBe(1); - expect(filteredFieldGroup[1].fields.length).toBe(1); + expect(filteredFieldGroup).toEqual([ + { fields: ["foo"] }, + { fields: ["name"] }, + ]); }); it("should remove controls with read-only permissions from form", async () => { From f99b8d0271d5ad22b932f6ca82c527eed0dd8af6 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Tue, 16 Jan 2024 09:03:27 +0100 Subject: [PATCH 09/20] fix: test syntax Co-authored-by: Simon --- .../common-components/entity-form/entity-form.service.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.spec.ts b/src/app/core/common-components/entity-form/entity-form.service.spec.ts index 79bc34cde4..3c709f76af 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.spec.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.spec.ts @@ -91,9 +91,9 @@ describe("EntityFormService", () => { service.disableReadOnlyFormControls(formGroup, entity); - expect(formGroup.get("name").disabled).toBeTruthy(); + expect(formGroup.get("name").disabled).toBeTrue(); expect(formGroup.get("foo").disabled).toBeFalse(); - expect(formGroup.get("bar").disabled).toBeTruthy(); + expect(formGroup.get("bar").disabled).toBeTrue(); }); it("should update entity if saving is successful", async () => { From 754bf3ea2db2d2dc91dd81bd6cbffa09c95fcecc Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Tue, 16 Jan 2024 11:29:36 +0100 Subject: [PATCH 10/20] fix: pr review feedback --- .../entity-form/entity-form.service.spec.ts | 26 +----------- .../entity-form/entity-form.service.ts | 25 +++-------- .../entity-form/entity-form.component.spec.ts | 34 +++++++++++++++ .../entity-form/entity-form.component.ts | 41 +++++++++++++------ .../entity-select.component.spec.ts | 4 -- .../form/form.component.spec.ts | 5 --- .../entity-list/entity-list.component.spec.ts | 4 -- .../ability/testing-entity-ability-factory.ts | 10 +++++ src/app/utils/mocked-testing.module.ts | 9 ++++ 9 files changed, 88 insertions(+), 70 deletions(-) create mode 100644 src/app/core/permissions/ability/testing-entity-ability-factory.ts diff --git a/src/app/core/common-components/entity-form/entity-form.service.spec.ts b/src/app/core/common-components/entity-form/entity-form.service.spec.ts index 3c709f76af..fa67a48662 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.spec.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.spec.ts @@ -26,7 +26,6 @@ import { DatabaseField } from "../../entity/database-field.decorator"; import { EntitySchemaService } from "../../entity/schema/entity-schema.service"; import { FormFieldConfig } from "./FormConfig"; import { TEST_USER } from "../../user/demo-user-generator.service"; -import { FieldGroup } from "../../entity-details/form/field-group"; describe("EntityFormService", () => { let service: EntityFormService; @@ -55,30 +54,7 @@ describe("EntityFormService", () => { expect(entity.getId()).not.toBe("newId"); }); - it("should remove fields without read permissions", async () => { - const entity = new Entity(); - const fieldGroup: FieldGroup[] = [ - { fields: ["foo", "bar"] }, - { fields: ["name"] }, - { fields: ["birthday"] }, - ]; - - TestBed.inject(EntityAbility).update([ - { subject: "Entity", action: "read", fields: ["foo", "name"] }, - ]); - - const filteredFieldGroup = service.filterFieldGroupsByPermissions( - fieldGroup, - entity, - ); - - expect(filteredFieldGroup).toEqual([ - { fields: ["foo"] }, - { fields: ["name"] }, - ]); - }); - - it("should remove controls with read-only permissions from form", async () => { + it("should remove controls with read-only permissions from form", () => { const entity = new Entity(); const formGroup = new UntypedFormGroup({ name: new UntypedFormControl("name"), diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index 317814df36..2fe41dae5a 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -18,7 +18,6 @@ import { } from "../../entity/schema/entity-schema-field"; import { isArrayDataType } from "../../basic-datatypes/datatype-utils"; import { CurrentUserSubject } from "../../session/current-user-subject"; -import { FieldGroup } from "../../entity-details/form/field-group"; /** * These are utility types that allow to define the type of `FormGroup` the way it is returned by `EntityFormService.create` @@ -79,24 +78,6 @@ export class EntityFormService { } } - filterFieldGroupsByPermissions( - fieldGroups: FieldGroup[], - entity: T, - ): FieldGroup[] { - return fieldGroups - .map((group) => { - group.fields = group.fields.filter((field) => - this.ability.can( - "read", - entity, - typeof field === "string" ? field : field.id, - ), - ); - return group; - }) - .filter((group) => group.fields.length > 0); - } - disableReadOnlyFormControls( form: EntityForm, entity: T, @@ -148,6 +129,7 @@ export class EntityFormService { formFields: ColumnConfig[], entity: T, forTable = false, + withPermissions = true, ): EntityForm { const formConfig = {}; const copy = entity.copy(); @@ -164,6 +146,11 @@ export class EntityFormService { const sub = group.valueChanges.subscribe( () => (this.unsavedChanges.pending = group.dirty), ); + + if (withPermissions) { + const disableSub = group.statusChanges.subscribe((value) => value); + } + this.subscriptions.push(sub); return group; diff --git a/src/app/core/common-components/entity-form/entity-form/entity-form.component.spec.ts b/src/app/core/common-components/entity-form/entity-form/entity-form.component.spec.ts index c57994f075..15db8be1fb 100644 --- a/src/app/core/common-components/entity-form/entity-form/entity-form.component.spec.ts +++ b/src/app/core/common-components/entity-form/entity-form/entity-form.component.spec.ts @@ -7,6 +7,7 @@ import { EntityMapperService } from "../../../entity/entity-mapper/entity-mapper import { ConfirmationDialogService } from "../../confirmation-dialog/confirmation-dialog.service"; import { EntityFormService } from "../entity-form.service"; import { DateWithAge } from "../../../basic-datatypes/date-with-age/dateWithAge"; +import { EntityAbility } from "../../../permissions/ability/entity-ability"; describe("EntityFormComponent", () => { let component: EntityFormComponent; @@ -55,6 +56,39 @@ describe("EntityFormComponent", () => { expect(component).toBeTruthy(); }); + it("should remove fields without read permissions", async () => { + component.fieldGroups = [ + { fields: ["foo", "bar"] }, + { fields: ["name"] }, + { fields: ["birthday"] }, + ]; + + TestBed.inject(EntityAbility).update([ + { subject: "Child", action: "read", fields: ["foo", "name"] }, + ]); + + component.ngOnChanges({ entity: true, form: true } as any); + + expect(component.fieldGroups).toEqual([ + { fields: ["foo"] }, + { fields: ["name"] }, + ]); + }); + + it("should disable read-only fields after form enable", async () => { + TestBed.inject(EntityAbility).update([ + { subject: "Child", action: "read", fields: ["projectNumber", "name"] }, + { subject: "Child", action: "update", fields: ["name"] }, + ]); + + component.ngOnChanges({ entity: true, form: true } as any); + + component.form.enable(); + + expect(component.form.get("name").enabled).toBeTrue(); + expect(component.form.get("projectNumber").disabled).toBeTrue(); + }); + it("should not change anything if changed entity has same values as form", () => { return expectApplyChangesPopup( "not-shown", diff --git a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts index 7c2f96186c..4830d3613f 100644 --- a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts +++ b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts @@ -1,10 +1,4 @@ -import { - Component, - Input, - OnChanges, - SimpleChanges, - ViewEncapsulation, -} from "@angular/core"; +import { Component, Input, OnChanges, SimpleChanges, ViewEncapsulation } from "@angular/core"; import { Entity } from "../../../entity/model/entity"; import { EntityForm, EntityFormService } from "../entity-form.service"; import { EntityMapperService } from "../../../entity/entity-mapper/entity-mapper.service"; @@ -16,6 +10,7 @@ import { Subscription } from "rxjs"; import moment from "moment"; import { EntityFieldEditComponent } from "../../entity-field-edit/entity-field-edit.component"; import { FieldGroup } from "../../../entity-details/form/field-group"; +import { EntityAbility } from "../../../permissions/ability/entity-ability"; /** * A general purpose form component for displaying and editing entities. @@ -61,6 +56,7 @@ export class EntityFormComponent private entityMapper: EntityMapperService, private confirmationDialog: ConfirmationDialogService, private entityFormService: EntityFormService, + private ability: EntityAbility, ) {} ngOnChanges(changes: SimpleChanges) { @@ -92,18 +88,19 @@ export class EntityFormComponent private applyFormFieldPermissions() { if (this.fieldGroups) { - this.fieldGroups = this.entityFormService.filterFieldGroupsByPermissions( + this.fieldGroups = this.filterFieldGroupsByPermissions( this.fieldGroups, this.entity, ); } if (this.form) { - this.form.statusChanges.pipe(untilDestroyed(this)).subscribe((_) => { - this.entityFormService.disableReadOnlyFormControls( - this.form, - this.entity, - ); + this.form.statusChanges.pipe(untilDestroyed(this)).subscribe((status) => { + if (status !== "DISABLED") + this.entityFormService.disableReadOnlyFormControls( + this.form, + this.entity, + ); }); } } @@ -153,6 +150,24 @@ export class EntityFormComponent ); } + private filterFieldGroupsByPermissions( + fieldGroups: FieldGroup[], + entity: , + ): FieldGroup[] { + return fieldGroups + .map((group) => { + group.fields = group.fields.filter((field) => + this.ability.can( + "read", + entity, + typeof field === "string" ? field : field.i, + , + ); + return group; + }) + .filter((group) => group.fields.length > 0); + } + private entityEqualsFormValue(entityValue, formValue) { return ( (entityValue instanceof Date && diff --git a/src/app/core/common-components/entity-select/entity-select.component.spec.ts b/src/app/core/common-components/entity-select/entity-select.component.spec.ts index 4b7157dc56..b474f8034c 100644 --- a/src/app/core/common-components/entity-select/entity-select.component.spec.ts +++ b/src/app/core/common-components/entity-select/entity-select.component.spec.ts @@ -12,12 +12,10 @@ import { Child } from "../../../child-dev-project/children/model/child"; import { School } from "../../../child-dev-project/schools/model/school"; import { MockedTestingModule } from "../../../utils/mocked-testing.module"; import { LoginState } from "../../session/session-states/login-state.enum"; -import { EntityAbility } from "../../permissions/ability/entity-ability"; describe("EntitySelectComponent", () => { let component: EntitySelectComponent; let fixture: ComponentFixture>; - let entityAbility: EntityAbility; const testUsers: Entity[] = ["Abc", "Bcd", "Abd", "Aba"].map((s) => { const user = new User(); @@ -38,8 +36,6 @@ describe("EntitySelectComponent", () => { ]), ], }).compileComponents(); - entityAbility = TestBed.inject(EntityAbility); - spyOn(entityAbility, "can").and.returnValue(true); })); beforeEach(() => { diff --git a/src/app/core/entity-details/form/form.component.spec.ts b/src/app/core/entity-details/form/form.component.spec.ts index eea9c0bb63..ecdf23af5d 100644 --- a/src/app/core/entity-details/form/form.component.spec.ts +++ b/src/app/core/entity-details/form/form.component.spec.ts @@ -7,21 +7,16 @@ import { MockedTestingModule } from "../../../utils/mocked-testing.module"; import { ConfirmationDialogService } from "../../common-components/confirmation-dialog/confirmation-dialog.service"; import { AlertService } from "../../alerts/alert.service"; import { EntityFormService } from "../../common-components/entity-form/entity-form.service"; -import { EntityAbility } from "../../permissions/ability/entity-ability"; describe("FormComponent", () => { let component: FormComponent; let fixture: ComponentFixture>; - let entityAbility: EntityAbility; - beforeEach(waitForAsync(() => { TestBed.configureTestingModule({ imports: [FormComponent, MockedTestingModule.withState()], providers: [{ provide: ConfirmationDialogService, useValue: null }], }).compileComponents(); - entityAbility = TestBed.inject(EntityAbility); - spyOn(entityAbility, "can").and.returnValue(true); })); beforeEach(() => { diff --git a/src/app/core/entity-list/entity-list/entity-list.component.spec.ts b/src/app/core/entity-list/entity-list/entity-list.component.spec.ts index de402a0a37..8288714b8d 100644 --- a/src/app/core/entity-list/entity-list/entity-list.component.spec.ts +++ b/src/app/core/entity-list/entity-list/entity-list.component.spec.ts @@ -21,13 +21,11 @@ import { TestbedHarnessEnvironment } from "@angular/cdk/testing/testbed"; import { MatTabGroupHarness } from "@angular/material/tabs/testing"; import { FormDialogService } from "../../form-dialog/form-dialog.service"; import { UpdatedEntity } from "../../entity/model/entity-update"; -import { EntityAbility } from "../../permissions/ability/entity-ability"; describe("EntityListComponent", () => { let component: EntityListComponent; let fixture: ComponentFixture>; let loader: HarnessLoader; - let entityAbility: EntityAbility; const testConfig: EntityListConfig = { title: "Children List", @@ -96,8 +94,6 @@ describe("EntityListComponent", () => { { provide: FormDialogService, useValue: null }, ], }).compileComponents(); - entityAbility = TestBed.inject(EntityAbility); - spyOn(entityAbility, "can").and.returnValue(true); })); it("should create", () => { diff --git a/src/app/core/permissions/ability/testing-entity-ability-factory.ts b/src/app/core/permissions/ability/testing-entity-ability-factory.ts new file mode 100644 index 0000000000..5425812b3a --- /dev/null +++ b/src/app/core/permissions/ability/testing-entity-ability-factory.ts @@ -0,0 +1,10 @@ +import { EntitySchemaService } from "../../entity/schema/entity-schema.service"; +import { EntityAbility } from "./entity-ability"; + +export const entityAbilityFactory = ( + entitySchemaService: EntitySchemaService, +) => { + let ability = new EntityAbility(entitySchemaService); + ability.update([{ subject: "all", action: "manage" }]); + return ability; +}; diff --git a/src/app/utils/mocked-testing.module.ts b/src/app/utils/mocked-testing.module.ts index 2f6c6e0a6f..c5414eef6a 100644 --- a/src/app/utils/mocked-testing.module.ts +++ b/src/app/utils/mocked-testing.module.ts @@ -23,6 +23,9 @@ import { BehaviorSubject } from "rxjs"; import { CurrentUserSubject } from "../core/session/current-user-subject"; import { SessionInfo, SessionSubject } from "../core/session/auth/session-info"; import { TEST_USER } from "../core/user/demo-user-generator.service"; +import { EntityAbility } from "../core/permissions/ability/entity-ability"; +import { EntitySchemaService } from "../core/entity/schema/entity-schema.service"; +import { entityAbilityFactory } from "app/core/permissions/ability/testing-entity-ability-factory"; /** * Utility module that can be imported in test files or stories to have mock implementations of the SessionService @@ -72,9 +75,15 @@ export class MockedTestingModule { ): ModuleWithProviders { environment.session_type = SessionType.mock; const mockedEntityMapper = mockEntityMapper([...data]); + return { ngModule: MockedTestingModule, providers: [ + { + provide: EntityAbility, + useFactory: entityAbilityFactory, + deps: [EntitySchemaService], + }, { provide: EntityMapperService, useValue: mockedEntityMapper }, { provide: ConfigService, useValue: createTestingConfigService() }, { From cf798729335f6af39804e414199f1a37a5530044 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Tue, 16 Jan 2024 11:33:29 +0100 Subject: [PATCH 11/20] fix: typo --- .../entity-form/entity-form.component.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts index 4830d3613f..d8f49c596a 100644 --- a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts +++ b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts @@ -1,4 +1,10 @@ -import { Component, Input, OnChanges, SimpleChanges, ViewEncapsulation } from "@angular/core"; +import { + Component, + Input, + OnChanges, + SimpleChanges, + ViewEncapsulation, +} from "@angular/core"; import { Entity } from "../../../entity/model/entity"; import { EntityForm, EntityFormService } from "../entity-form.service"; import { EntityMapperService } from "../../../entity/entity-mapper/entity-mapper.service"; @@ -152,7 +158,7 @@ export class EntityFormComponent private filterFieldGroupsByPermissions( fieldGroups: FieldGroup[], - entity: , + entity: Entit, ): FieldGroup[] { return fieldGroups .map((group) => { @@ -160,8 +166,8 @@ export class EntityFormComponent this.ability.can( "read", entity, - typeof field === "string" ? field : field.i, - , + typeof field === "string" ? field : field.id, + ), ); return group; }) From b1feafc5cd4acf9cb1ff176ba94bbd7a04fe619a Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Tue, 16 Jan 2024 11:34:22 +0100 Subject: [PATCH 12/20] fix: typo --- .../entity-form/entity-form/entity-form.component.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts index d8f49c596a..baf442a7a4 100644 --- a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts +++ b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts @@ -158,7 +158,7 @@ export class EntityFormComponent private filterFieldGroupsByPermissions( fieldGroups: FieldGroup[], - entity: Entit, + entity: Entity, ): FieldGroup[] { return fieldGroups .map((group) => { From c2290c8635ba73dbd85ff57a3635563b2e1bb548 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Tue, 16 Jan 2024 11:45:08 +0100 Subject: [PATCH 13/20] fix: pr review feedback --- .../entity-form/entity-form.service.ts | 17 ++++++++++++----- .../entity-form/entity-form.component.ts | 10 ---------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index 2fe41dae5a..7d25ad594d 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -124,12 +124,13 @@ export class EntityFormService { * @param formFields * @param entity * @param forTable + * @param withPermissionCheck enable permission based field filter */ public createFormGroup( formFields: ColumnConfig[], entity: T, forTable = false, - withPermissions = true, + withPermissionCheck = true, ): EntityForm { const formConfig = {}; const copy = entity.copy(); @@ -143,15 +144,21 @@ export class EntityFormService { } const group = this.fb.group>(formConfig); - const sub = group.valueChanges.subscribe( + const valueChangesSubscription = group.valueChanges.subscribe( () => (this.unsavedChanges.pending = group.dirty), ); - if (withPermissions) { - const disableSub = group.statusChanges.subscribe((value) => value); + if (withPermissionCheck) { + const statusChangesSubscription = group.statusChanges.subscribe( + (status) => { + if (status !== "DISABLED") + this.disableReadOnlyFormControls(group, entity); + }, + ); + this.subscriptions.push(statusChangesSubscription); } - this.subscriptions.push(sub); + this.subscriptions.push(valueChangesSubscription); return group; } diff --git a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts index baf442a7a4..23015afa30 100644 --- a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts +++ b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts @@ -99,16 +99,6 @@ export class EntityFormComponent this.entity, ); } - - if (this.form) { - this.form.statusChanges.pipe(untilDestroyed(this)).subscribe((status) => { - if (status !== "DISABLED") - this.entityFormService.disableReadOnlyFormControls( - this.form, - this.entity, - ); - }); - } } private async applyChanges(externallyUpdatedEntity: T) { From b6923215113117ffc7795642db1ff126fb0f434c Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Thu, 18 Jan 2024 19:52:53 +0100 Subject: [PATCH 14/20] fix: remove unnecessary function call --- .../entity-form/entity-form.service.spec.ts | 38 ++++++++++--------- .../entity-form/entity-form.service.ts | 7 +--- .../entity-form/entity-form.component.spec.ts | 14 ------- .../entity-form/entity-form.component.ts | 35 +++++------------ 4 files changed, 31 insertions(+), 63 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.spec.ts b/src/app/core/common-components/entity-form/entity-form.service.spec.ts index fa67a48662..9007067de1 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.spec.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.spec.ts @@ -54,24 +54,6 @@ describe("EntityFormService", () => { expect(entity.getId()).not.toBe("newId"); }); - it("should remove controls with read-only permissions from form", () => { - const entity = new Entity(); - const formGroup = new UntypedFormGroup({ - name: new UntypedFormControl("name"), - foo: new UntypedFormControl("foo"), - bar: new UntypedFormControl("bar"), - }); - TestBed.inject(EntityAbility).update([ - { subject: "Entity", action: "update", fields: ["foo"] }, - ]); - - service.disableReadOnlyFormControls(formGroup, entity); - - expect(formGroup.get("name").disabled).toBeTrue(); - expect(formGroup.get("foo").disabled).toBeFalse(); - expect(formGroup.get("bar").disabled).toBeTrue(); - }); - it("should update entity if saving is successful", async () => { const entity = new Entity("initialId"); const formGroup = new UntypedFormGroup({ @@ -126,6 +108,26 @@ describe("EntityFormService", () => { expect(formGroup.valid).toBeTrue(); }); + it("should disable read-only fields after form enable", () => { + const formFields = [{ id: "name" }, { id: "dateOfBirth" }]; + const formGroup = service.createFormGroup(formFields, new Child()); + + TestBed.inject(EntityAbility).update([ + { subject: "Child", action: "read", fields: ["name", "dateOfBirth"] }, + { subject: "Child", action: "update", fields: ["name"] }, + ]); + + formGroup.disable(); + + expect(formGroup.get("name").disabled).toBeTrue(); + expect(formGroup.get("dateOfBirth").disabled).toBeTrue(); + + formGroup.enable(); + + expect(formGroup.get("name").enabled).toBeTrue(); + expect(formGroup.get("dateOfBirth").disabled).toBeTrue(); + }); + it("should create a error if form is invalid", () => { const formFields = [{ id: "schoolId" }, { id: "start" }]; const formGroup = service.createFormGroup( diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index 7d25ad594d..ea17a058b9 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -12,10 +12,7 @@ import { UnsavedChangesService } from "../../entity-details/form/unsaved-changes import { ActivationStart, Router } from "@angular/router"; import { Subscription } from "rxjs"; import { filter } from "rxjs/operators"; -import { - EntitySchemaField, - PLACEHOLDERS, -} from "../../entity/schema/entity-schema-field"; +import { EntitySchemaField, PLACEHOLDERS } from "../../entity/schema/entity-schema-field"; import { isArrayDataType } from "../../basic-datatypes/datatype-utils"; import { CurrentUserSubject } from "../../session/current-user-subject"; @@ -78,7 +75,7 @@ export class EntityFormService { } } - disableReadOnlyFormControls( + private disableReadOnlyFormControls( form: EntityForm, entity: T, ) { diff --git a/src/app/core/common-components/entity-form/entity-form/entity-form.component.spec.ts b/src/app/core/common-components/entity-form/entity-form/entity-form.component.spec.ts index 15db8be1fb..b340b42772 100644 --- a/src/app/core/common-components/entity-form/entity-form/entity-form.component.spec.ts +++ b/src/app/core/common-components/entity-form/entity-form/entity-form.component.spec.ts @@ -75,20 +75,6 @@ describe("EntityFormComponent", () => { ]); }); - it("should disable read-only fields after form enable", async () => { - TestBed.inject(EntityAbility).update([ - { subject: "Child", action: "read", fields: ["projectNumber", "name"] }, - { subject: "Child", action: "update", fields: ["name"] }, - ]); - - component.ngOnChanges({ entity: true, form: true } as any); - - component.form.enable(); - - expect(component.form.get("name").enabled).toBeTrue(); - expect(component.form.get("projectNumber").disabled).toBeTrue(); - }); - it("should not change anything if changed entity has same values as form", () => { return expectApplyChangesPopup( "not-shown", diff --git a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts index 23015afa30..e265ed5d4f 100644 --- a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts +++ b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts @@ -1,12 +1,6 @@ -import { - Component, - Input, - OnChanges, - SimpleChanges, - ViewEncapsulation, -} from "@angular/core"; +import { Component, Input, OnChanges, SimpleChanges, ViewEncapsulation } from "@angular/core"; import { Entity } from "../../../entity/model/entity"; -import { EntityForm, EntityFormService } from "../entity-form.service"; +import { EntityForm } from "../entity-form.service"; import { EntityMapperService } from "../../../entity/entity-mapper/entity-mapper.service"; import { filter } from "rxjs/operators"; import { ConfirmationDialogService } from "../../confirmation-dialog/confirmation-dialog.service"; @@ -61,12 +55,16 @@ export class EntityFormComponent constructor( private entityMapper: EntityMapperService, private confirmationDialog: ConfirmationDialogService, - private entityFormService: EntityFormService, private ability: EntityAbility, ) {} ngOnChanges(changes: SimpleChanges) { - this.applyFormFieldPermissions(); + if (this.fieldGroups) { + this.fieldGroups = this.filterFieldGroupsByPermissions( + this.fieldGroups, + this.entity, + ); + } if (changes.entity && this.entity) { this.changesSubscription?.unsubscribe(); @@ -79,26 +77,11 @@ export class EntityFormComponent ) .subscribe(({ entity }) => this.applyChanges(entity)); } + if (changes.form && this.form) { this.initialFormValues = this.form.getRawValue(); this.disableForLockedEntity(); } - - if (this.form && this.entity) { - this.entityFormService.disableReadOnlyFormControls( - this.form, - this.entity, - ); - } - } - - private applyFormFieldPermissions() { - if (this.fieldGroups) { - this.fieldGroups = this.filterFieldGroupsByPermissions( - this.fieldGroups, - this.entity, - ); - } } private async applyChanges(externallyUpdatedEntity: T) { From 81422225fd3e88880ae7d9f292b678a5c49531ed Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Thu, 18 Jan 2024 20:00:42 +0100 Subject: [PATCH 15/20] fix: code format --- .../entity-form/entity-form/entity-form.component.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts index e265ed5d4f..81ab7eb210 100644 --- a/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts +++ b/src/app/core/common-components/entity-form/entity-form/entity-form.component.ts @@ -1,4 +1,10 @@ -import { Component, Input, OnChanges, SimpleChanges, ViewEncapsulation } from "@angular/core"; +import { + Component, + Input, + OnChanges, + SimpleChanges, + ViewEncapsulation, +} from "@angular/core"; import { Entity } from "../../../entity/model/entity"; import { EntityForm } from "../entity-form.service"; import { EntityMapperService } from "../../../entity/entity-mapper/entity-mapper.service"; From dd2f5ff60632268fcfdd90e5fa92dd178b5e710f Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Thu, 18 Jan 2024 20:07:18 +0100 Subject: [PATCH 16/20] fix: code format --- .../common-components/entity-form/entity-form.service.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index ea17a058b9..4b3a889d2c 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -12,7 +12,10 @@ import { UnsavedChangesService } from "../../entity-details/form/unsaved-changes import { ActivationStart, Router } from "@angular/router"; import { Subscription } from "rxjs"; import { filter } from "rxjs/operators"; -import { EntitySchemaField, PLACEHOLDERS } from "../../entity/schema/entity-schema-field"; +import { + EntitySchemaField, + PLACEHOLDERS, +} from "../../entity/schema/entity-schema-field"; import { isArrayDataType } from "../../basic-datatypes/datatype-utils"; import { CurrentUserSubject } from "../../session/current-user-subject"; From baecf55887d6c31a353244828e2f19cebf71ed45 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 22 Jan 2024 11:26:11 +0100 Subject: [PATCH 17/20] disabled check also happens on form initialization --- .../entity-form/entity-form.service.spec.ts | 9 +++-- .../entity-form/entity-form.service.ts | 37 +++++++++---------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.spec.ts b/src/app/core/common-components/entity-form/entity-form.service.spec.ts index 9007067de1..78e65a2775 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.spec.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.spec.ts @@ -108,15 +108,18 @@ describe("EntityFormService", () => { expect(formGroup.valid).toBeTrue(); }); - it("should disable read-only fields after form enable", () => { + it("should always keep properties disabled if user does not have 'update' permissions for them", () => { const formFields = [{ id: "name" }, { id: "dateOfBirth" }]; - const formGroup = service.createFormGroup(formFields, new Child()); - TestBed.inject(EntityAbility).update([ { subject: "Child", action: "read", fields: ["name", "dateOfBirth"] }, { subject: "Child", action: "update", fields: ["name"] }, ]); + const formGroup = service.createFormGroup(formFields, new Child()); + + expect(formGroup.get("name").enabled).toBeTrue(); + expect(formGroup.get("dateOfBirth").disabled).toBeTrue(); + formGroup.disable(); expect(formGroup.get("name").disabled).toBeTrue(); diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index 4b3a889d2c..bdb550c6ee 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -78,17 +78,6 @@ export class EntityFormService { } } - private disableReadOnlyFormControls( - form: EntityForm, - entity: T, - ) { - Object.keys(form.controls).forEach((fieldId) => { - if (!this.ability.can("update", entity, fieldId)) { - form.get(fieldId).disable({ onlySelf: true, emitEvent: false }); - } - }); - } - private addSchemaToFormField( formField: FormFieldConfig, propertySchema: EntitySchemaField, @@ -124,7 +113,7 @@ export class EntityFormService { * @param formFields * @param entity * @param forTable - * @param withPermissionCheck enable permission based field filter + * @param withPermissionCheck if true, fields without 'update' permissions will stay disabled when enabling form */ public createFormGroup( formFields: ColumnConfig[], @@ -147,19 +136,16 @@ export class EntityFormService { const valueChangesSubscription = group.valueChanges.subscribe( () => (this.unsavedChanges.pending = group.dirty), ); + this.subscriptions.push(valueChangesSubscription); if (withPermissionCheck) { - const statusChangesSubscription = group.statusChanges.subscribe( - (status) => { - if (status !== "DISABLED") - this.disableReadOnlyFormControls(group, entity); - }, - ); + this.disableReadOnlyFormControls(group, entity); + const statusChangesSubscription = group.statusChanges + .pipe(filter((status) => status !== "DISABLED")) + .subscribe(() => this.disableReadOnlyFormControls(group, entity)); this.subscriptions.push(statusChangesSubscription); } - this.subscriptions.push(valueChangesSubscription); - return group; } @@ -219,6 +205,17 @@ export class EntityFormService { return newVal; } + private disableReadOnlyFormControls( + form: EntityForm, + entity: T, + ) { + Object.keys(form.controls).forEach((fieldId) => { + if (this.ability.cannot("update", entity, fieldId)) { + form.get(fieldId).disable({ onlySelf: true, emitEvent: false }); + } + }); + } + /** * This function applies the changes of the formGroup to the entity. * If the form is invalid or the entity does not pass validation after applying the changes, an error will be thrown. From 429428d5b2d793a929a3a3d057fa04bbfbb1aa99 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Wed, 24 Jan 2024 15:51:07 +0100 Subject: [PATCH 18/20] fix: use create permissions when form creates new entity --- .../entity-form/entity-form.service.spec.ts | 20 +++++++++++++++++++ .../entity-form/entity-form.service.ts | 12 ++++++++--- .../entity-details/form/form.component.ts | 3 +++ 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.spec.ts b/src/app/core/common-components/entity-form/entity-form.service.spec.ts index 40ddb0016f..d27623946e 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.spec.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.spec.ts @@ -108,6 +108,26 @@ describe("EntityFormService", () => { expect(formGroup.valid).toBeTrue(); }); + it("should use create permissions to disable fields when creating a new entity", () => { + const formFields = [{ id: "name" }, { id: "dateOfBirth" }]; + TestBed.inject(EntityAbility).update([ + { subject: "Child", action: "read", fields: ["name", "dateOfBirth"] }, + { subject: "Child", action: "update", fields: ["name"] }, + { subject: "Child", action: "create", fields: ["dateOfBirth"] }, + ]); + + const formGroup = service.createFormGroup( + formFields, + new Child(), + false, + true, + true, + ); + + expect(formGroup.get("name").disabled).toBeTrue(); + expect(formGroup.get("dateOfBirth").enabled).toBeTrue(); + }); + it("should always keep properties disabled if user does not have 'update' permissions for them", () => { const formFields = [{ id: "name" }, { id: "dateOfBirth" }]; TestBed.inject(EntityAbility).update([ diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index 08aa76822e..0ea8b5e197 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -113,12 +113,14 @@ export class EntityFormService { * @param entity * @param forTable * @param withPermissionCheck if true, fields without 'update' permissions will stay disabled when enabling form + * @param creatingNew if true, fields without 'create' permissions will stay disabled initially */ public createFormGroup( formFields: ColumnConfig[], entity: T, forTable = false, withPermissionCheck = true, + creatingNew = false, ): EntityForm { const formConfig = {}; const copy = entity.copy(); @@ -138,10 +140,12 @@ export class EntityFormService { this.subscriptions.push(valueChangesSubscription); if (withPermissionCheck) { - this.disableReadOnlyFormControls(group, entity); + this.disableReadOnlyFormControls(group, entity, creatingNew); const statusChangesSubscription = group.statusChanges .pipe(filter((status) => status !== "DISABLED")) - .subscribe(() => this.disableReadOnlyFormControls(group, entity)); + .subscribe(() => + this.disableReadOnlyFormControls(group, entity, creatingNew), + ); this.subscriptions.push(statusChangesSubscription); } @@ -207,9 +211,11 @@ export class EntityFormService { private disableReadOnlyFormControls( form: EntityForm, entity: T, + creatingNew: boolean, ) { + const action = creatingNew ? "create" : "update"; Object.keys(form.controls).forEach((fieldId) => { - if (this.ability.cannot("update", entity, fieldId)) { + if (this.ability.cannot(action, entity, fieldId)) { form.get(fieldId).disable({ onlySelf: true, emitEvent: false }); } }); diff --git a/src/app/core/entity-details/form/form.component.ts b/src/app/core/entity-details/form/form.component.ts index 1071f27b70..2745672971 100644 --- a/src/app/core/entity-details/form/form.component.ts +++ b/src/app/core/entity-details/form/form.component.ts @@ -51,6 +51,9 @@ export class FormComponent implements FormConfig, OnInit { this.form = this.entityFormService.createFormGroup( [].concat(...this.fieldGroups.map((group) => group.fields)), this.entity, + false, + true, + this.creatingNew, ); if (!this.creatingNew) { From 28d6bdb4030d4be03d84099f588fe496165c1c88 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Wed, 24 Jan 2024 16:40:45 +0100 Subject: [PATCH 19/20] fix: use create permissions when form creates new entity --- .../entity-form/entity-form.service.spec.ts | 13 +++++-------- .../entity-form/entity-form.service.ts | 6 ++---- src/app/core/entity-details/form/form.component.ts | 3 --- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.spec.ts b/src/app/core/common-components/entity-form/entity-form.service.spec.ts index d27623946e..3a04a2b807 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.spec.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.spec.ts @@ -116,13 +116,7 @@ describe("EntityFormService", () => { { subject: "Child", action: "create", fields: ["dateOfBirth"] }, ]); - const formGroup = service.createFormGroup( - formFields, - new Child(), - false, - true, - true, - ); + const formGroup = service.createFormGroup(formFields, new Child()); expect(formGroup.get("name").disabled).toBeTrue(); expect(formGroup.get("dateOfBirth").enabled).toBeTrue(); @@ -135,7 +129,10 @@ describe("EntityFormService", () => { { subject: "Child", action: "update", fields: ["name"] }, ]); - const formGroup = service.createFormGroup(formFields, new Child()); + const child = new Child(); + child._rev = "foo"; // "not new" state + + const formGroup = service.createFormGroup(formFields, child); expect(formGroup.get("name").enabled).toBeTrue(); expect(formGroup.get("dateOfBirth").disabled).toBeTrue(); diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index 0ea8b5e197..4eea15890b 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -113,14 +113,12 @@ export class EntityFormService { * @param entity * @param forTable * @param withPermissionCheck if true, fields without 'update' permissions will stay disabled when enabling form - * @param creatingNew if true, fields without 'create' permissions will stay disabled initially */ public createFormGroup( formFields: ColumnConfig[], entity: T, forTable = false, withPermissionCheck = true, - creatingNew = false, ): EntityForm { const formConfig = {}; const copy = entity.copy(); @@ -140,11 +138,11 @@ export class EntityFormService { this.subscriptions.push(valueChangesSubscription); if (withPermissionCheck) { - this.disableReadOnlyFormControls(group, entity, creatingNew); + this.disableReadOnlyFormControls(group, entity, entity.isNew); const statusChangesSubscription = group.statusChanges .pipe(filter((status) => status !== "DISABLED")) .subscribe(() => - this.disableReadOnlyFormControls(group, entity, creatingNew), + this.disableReadOnlyFormControls(group, entity, entity.isNew), ); this.subscriptions.push(statusChangesSubscription); } diff --git a/src/app/core/entity-details/form/form.component.ts b/src/app/core/entity-details/form/form.component.ts index 2745672971..1071f27b70 100644 --- a/src/app/core/entity-details/form/form.component.ts +++ b/src/app/core/entity-details/form/form.component.ts @@ -51,9 +51,6 @@ export class FormComponent implements FormConfig, OnInit { this.form = this.entityFormService.createFormGroup( [].concat(...this.fieldGroups.map((group) => group.fields)), this.entity, - false, - true, - this.creatingNew, ); if (!this.creatingNew) { From 587ff542e5d703147152da7b67022637c812afd4 Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 25 Jan 2024 10:28:28 +0100 Subject: [PATCH 20/20] removed unnecessary variable --- .../common-components/entity-form/entity-form.service.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index 4eea15890b..85526a3975 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -138,12 +138,10 @@ export class EntityFormService { this.subscriptions.push(valueChangesSubscription); if (withPermissionCheck) { - this.disableReadOnlyFormControls(group, entity, entity.isNew); + this.disableReadOnlyFormControls(group, entity); const statusChangesSubscription = group.statusChanges .pipe(filter((status) => status !== "DISABLED")) - .subscribe(() => - this.disableReadOnlyFormControls(group, entity, entity.isNew), - ); + .subscribe(() => this.disableReadOnlyFormControls(group, entity)); this.subscriptions.push(statusChangesSubscription); } @@ -209,9 +207,8 @@ export class EntityFormService { private disableReadOnlyFormControls( form: EntityForm, entity: T, - creatingNew: boolean, ) { - const action = creatingNew ? "create" : "update"; + const action = entity.isNew ? "create" : "update"; Object.keys(form.controls).forEach((fieldId) => { if (this.ability.cannot(action, entity, fieldId)) { form.get(fieldId).disable({ onlySelf: true, emitEvent: false });