From c5dc1888ea27cae87d5ff69a9e4581fe2c425d8b Mon Sep 17 00:00:00 2001 From: Sebastian Stehle Date: Wed, 24 Oct 2018 21:17:21 +0200 Subject: [PATCH] Bugfix in include method. --- .../Security/Permission.cs | 7 +- .../app/framework/utils/permission.spec.ts | 105 ++++++++++++------ src/Squidex/app/framework/utils/permission.ts | 63 ++++++++--- .../shared/components/permission.directive.ts | 37 ++++-- .../shell/pages/app/left-menu.component.html | 2 +- .../internal/profile-menu.component.html | 2 +- .../Security/PermissionSetTests.cs | 16 ++- .../Security/PermissionTests.cs | 33 ++---- 8 files changed, 171 insertions(+), 94 deletions(-) diff --git a/src/Squidex.Infrastructure/Security/Permission.cs b/src/Squidex.Infrastructure/Security/Permission.cs index d2ba4801a..19ebd77e0 100644 --- a/src/Squidex.Infrastructure/Security/Permission.cs +++ b/src/Squidex.Infrastructure/Security/Permission.cs @@ -87,12 +87,7 @@ namespace Squidex.Infrastructure.Security return false; } - if (idParts.Length < permission.idParts.Length) - { - return false; - } - - for (var i = 0; i < permission.idParts.Length; i++) + for (var i = 0; i < Math.Min(idParts.Length, permission.idParts.Length); i++) { var lhs = idParts[i]; var rhs = permission.idParts[i]; diff --git a/src/Squidex/app/framework/utils/permission.spec.ts b/src/Squidex/app/framework/utils/permission.spec.ts index 8fb92f06b..190d59df9 100644 --- a/src/Squidex/app/framework/utils/permission.spec.ts +++ b/src/Squidex/app/framework/utils/permission.spec.ts @@ -5,102 +5,141 @@ * Copyright (c) Squidex UG (haftungsbeschränkt). All rights reserved. */ -import { Permission, permissionsAllow } from './permission'; +import { Permission } from './permission'; describe('Permission', () => { - it('Should_return_true_if_given_and_requested_permission_have_wildcards', () => { + it('should check when permissions are not equal', () => { + const g = new Permission('app.contents'); + const r = new Permission('app.assets'); + + expect(g.allows(r)).toBeFalsy(); + + expect(g.includes(r)).toBeFalsy(); + }); + + it('should check when permissions are equal with wildcards', () => { const g = new Permission('app.*'); const r = new Permission('app.*'); expect(g.allows(r)).toBeTruthy(); + + expect(g.includes(r)).toBeTruthy(); }); - it('Should_return_true_if_given_permission_equals_requested_permission', () => { + it('should check when equal permissions', () => { const g = new Permission('app.contents'); const r = new Permission('app.contents'); expect(g.allows(r)).toBeTruthy(); + + expect(g.includes(r)).toBeTruthy(); }); - it('Should_return_true_if_given_permission_is_parent_of_requested_permission', () => { + it('should check when given is parent of requested', () => { const g = new Permission('app'); const r = new Permission('app.contents'); expect(g.allows(r)).toBeTruthy(); - }); - - it('Should_return_true_if_given_permission_is_alternative_of_requested_permission', () => { - const g = new Permission('app.contents|schemas'); - const r = new Permission('app.contents'); - expect(g.allows(r)).toBeTruthy(); + expect(g.includes(r)).toBeTruthy(); }); - it('Should_return_true_if_given_permission_equals_alternative_requested_permission', () => { + it('should check when requested is parent of given', () => { const g = new Permission('app.contents'); - const r = new Permission('app.contents|schemas'); + const r = new Permission('app'); - expect(g.allows(r)).toBeTruthy(); + expect(g.allows(r)).toBeFalsy(); + + expect(g.includes(r)).toBeTruthy(); }); - it('Should_return_true_if_given_permission_has_wildcard_for_requested_permission', () => { + it('should check when given is wildcard of requested', () => { const g = new Permission('app.*'); const r = new Permission('app.contents'); expect(g.allows(r)).toBeTruthy(); + + expect(g.includes(r)).toBeTruthy(); }); - it('Should_return_false_if_given_permission_not_equals_requested_permission', () => { + it('should check when requested is wildcard of given', () => { const g = new Permission('app.contents'); - const r = new Permission('app.assets'); + const r = new Permission('app.*'); expect(g.allows(r)).toBeFalsy(); + + expect(g.includes(r)).toBeTruthy(); }); - it('Should_return_false_if_given_permission_is_child_of_requested_permission', () => { - const g = new Permission('app.contents'); - const r = new Permission('app'); + it('should check when given is has alternatives of requested', () => { + const g = new Permission('app.contents|schemas'); + const r = new Permission('app.contents'); - expect(g.allows(r)).toBeFalsy(); + expect(g.allows(r)).toBeTruthy(); + + expect(g.includes(r)).toBeTruthy(); }); - it('Should_return_false_if_given_permission_has_no_wildcard_but_requested_has', () => { + it('should check when requested is has alternatives of given', () => { const g = new Permission('app.contents'); - const r = new Permission('app.*'); + const r = new Permission('app.contents|schemas'); - expect(g.allows(r)).toBeFalsy(); + expect(g.allows(r)).toBeTruthy(); + + expect(g.includes(r)).toBeTruthy(); }); - it('Should_return_false_if_given_requested_permission_is_null', () => { + + it('should check for requested is null', () => { const g = new Permission('app.contents'); expect(g.allows(null!)).toBeFalsy(); + + expect(g.includes(null!)).toBeFalsy(); + }); + + it('should return true if any permission gives permission to requested', () => { + const set = [ + new Permission('app.contents'), + new Permission('app.assets') + ]; + + expect(new Permission('app.contents').allowedBy(set)).toBeTruthy(); + }); + + it('should return true if any permission includes parent given', () => { + const set = [ + new Permission('app.contents'), + new Permission('app.assets') + ]; + + expect(new Permission('app').includedIn(set)).toBeTruthy(); }); - it('Should_return_true_if_any_permission_gives_permission_to_request', () => { - const sut = [ + it('should return true if any permission includes child given', () => { + const set = [ new Permission('app.contents'), new Permission('app.assets') ]; - expect(permissionsAllow(sut, new Permission('app.contents'))).toBeTruthy(); + expect(new Permission('app.contents.read').includedIn(set)).toBeTruthy(); }); - it('Should_return_false_if_none_permission_gives_permission_to_request', () => { - const sut = [ + it('should return false if none permission gives permission to requested', () => { + const set = [ new Permission('app.contents'), new Permission('app.assets') ]; - expect(permissionsAllow(sut, new Permission('app.schemas'))).toBeFalsy(); + expect(new Permission('app.schemas').allowedBy(set)).toBeFalsy(); }); - it('Should_return_false_if_permission_to_request_is_null', () => { - const sut = [ + it('should return false if none permission includes given', () => { + const set = [ new Permission('app.contents'), new Permission('app.assets') ]; - expect(permissionsAllow(sut, null!)).toBeFalsy(); + expect(new Permission('other').allowedBy(set)).toBeFalsy(); }); }); \ No newline at end of file diff --git a/src/Squidex/app/framework/utils/permission.ts b/src/Squidex/app/framework/utils/permission.ts index 53c299bf9..f199709c5 100644 --- a/src/Squidex/app/framework/utils/permission.ts +++ b/src/Squidex/app/framework/utils/permission.ts @@ -28,6 +28,47 @@ export class Permission { }); } + public includedIn(permissions: Permission[]) { + for (let permission of permissions) { + if (permission.includes(this)) { + return true; + } + } + + return false; + } + + public allowedBy(permissions: Permission[]) { + for (let permission of permissions) { + if (permission.allows(this)) { + return true; + } + } + + return false; + } + + public includes(permission?: Permission | string) { + if (!permission) { + return false; + } + + if (Types.isString(permission)) { + permission = new Permission(permission); + } + + for (let i = 0; i < Math.min(permission.parts.length, this.parts.length); i++) { + const lhs = this.parts[i]; + const rhs = permission.parts[i]; + + if (lhs != null && rhs != null && !Permission.intersects(lhs, rhs)) { + return false; + } + } + + return true; + } + public allows(permission?: Permission | string) { if (!permission) { return false; @@ -42,10 +83,10 @@ export class Permission { } for (let i = 0; i < this.parts.length; i++) { - let lhs = this.parts[i]; - let rhs = permission.parts[i]; + const lhs = this.parts[i]; + const rhs = permission.parts[i]; - if (lhs !== null && (rhs === null || !Permission.any(lhs, rhs))) { + if (lhs !== null && (rhs === null || !Permission.intersects(lhs, rhs))) { return false; } } @@ -53,7 +94,7 @@ export class Permission { return true; } - private static any(lhs: { [key: string]: true }, rhs: { [key: string]: true }) { + private static intersects(lhs: { [key: string]: true }, rhs: { [key: string]: true }) { for (let key in lhs) { if (rhs[key]) { return true; @@ -68,18 +109,4 @@ export class Permission { return false; } -} - -export function permissionsAllow(permissions: Permission[], other: Permission) { - if (!other) { - return false; - } - - for (let permission of permissions) { - if (permission.allows(other)) { - return true; - } - } - - return false; } \ No newline at end of file diff --git a/src/Squidex/app/shared/components/permission.directive.ts b/src/Squidex/app/shared/components/permission.directive.ts index 42f115b84..61f92a175 100644 --- a/src/Squidex/app/shared/components/permission.directive.ts +++ b/src/Squidex/app/shared/components/permission.directive.ts @@ -12,7 +12,6 @@ import { AppsState, AuthService, Permission, - permissionsAllow, SchemaDto, SchemasState } from '@app/shared/internal'; @@ -42,10 +41,19 @@ export class PermissionDirective implements OnChanges { } public ngOnChanges() { + let permissions = this.permissions; let show = false; - if (this.permissions) { - for (let id of this.permissions.split(';')) { + if (permissions) { + let include = permissions[0] === '?'; + + if (include) { + permissions = permissions.substr(1); + } + + const array = permissions.split(';'); + + for (let id of array) { const app = this.app || this.appsState.snapshot.selectedApp; if (app) { @@ -60,12 +68,22 @@ export class PermissionDirective implements OnChanges { const permission = new Permission(id); - if (app && permissionsAllow(app.permissions, permission)) { - show = true; - } - - if (!show) { - show = permissionsAllow(this.authService.user!.permissions, permission); + if (include) { + if (app && permission.includedIn(app.permissions)) { + show = true; + } + + if (!show) { + show = permission.includedIn(this.authService.user!.permissions); + } + } else { + if (app && permission.allowedBy(app.permissions)) { + show = true; + } + + if (!show) { + show = permission.allowedBy(this.authService.user!.permissions); + } } if (show) { @@ -81,6 +99,5 @@ export class PermissionDirective implements OnChanges { this.viewContainer.clear(); this.viewCreated = false; } - } } \ No newline at end of file diff --git a/src/Squidex/app/shell/pages/app/left-menu.component.html b/src/Squidex/app/shell/pages/app/left-menu.component.html index 5e45ab974..3a632f819 100644 --- a/src/Squidex/app/shell/pages/app/left-menu.component.html +++ b/src/Squidex/app/shell/pages/app/left-menu.component.html @@ -19,7 +19,7 @@
Rules
-
  • +
  • Settings
     diff --git a/src/Squidex/app/shell/pages/internal/profile-menu.component.html b/src/Squidex/app/shell/pages/internal/profile-menu.component.html index 55bbd6c9e..c6e05c3ff 100644 --- a/src/Squidex/app/shell/pages/internal/profile-menu.component.html +++ b/src/Squidex/app/shell/pages/internal/profile-menu.component.html @@ -9,7 +9,7 @@
    - + Administration diff --git a/tests/Squidex.Infrastructure.Tests/Security/PermissionSetTests.cs b/tests/Squidex.Infrastructure.Tests/Security/PermissionSetTests.cs index 1e1684698..dbff3d2ac 100644 --- a/tests/Squidex.Infrastructure.Tests/Security/PermissionSetTests.cs +++ b/tests/Squidex.Infrastructure.Tests/Security/PermissionSetTests.cs @@ -35,7 +35,7 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_return_true_if_any_permission_gives_permission_to_request() + public void Should_return_true_if_any_permission_gives_permission_to_requested() { var sut = new PermissionSet( new Permission("app.contents"), @@ -45,7 +45,7 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_return_true_if_any_permission_includes_given() + public void Should_return_true_if_any_permission_includes_parent_given() { var sut = new PermissionSet( new Permission("app.contents"), @@ -55,7 +55,17 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_return_false_if_none_permission_gives_permission_to_request() + public void Should_return_true_if_any_permission_includes_child_given() + { + var sut = new PermissionSet( + new Permission("app.contents"), + new Permission("app.assets")); + + Assert.True(sut.Includes(new Permission("app.contents.read"))); + } + + [Fact] + public void Should_return_false_if_none_permission_gives_permission_to_requested() { var sut = new PermissionSet( new Permission("app.contents"), diff --git a/tests/Squidex.Infrastructure.Tests/Security/PermissionTests.cs b/tests/Squidex.Infrastructure.Tests/Security/PermissionTests.cs index 9eb8530fd..e04fbe623 100644 --- a/tests/Squidex.Infrastructure.Tests/Security/PermissionTests.cs +++ b/tests/Squidex.Infrastructure.Tests/Security/PermissionTests.cs @@ -24,7 +24,7 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_check_for_non_equal_wildcard_permissions() + public void Should_check_when_permissions_are_not_equal() { var g = new Permission("app.contents"); var r = new Permission("app.assets"); @@ -35,7 +35,7 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_check_for_equal_wildcard_permissions() + public void Should_check_when_permissions_are_equal_with_wildcards() { var g = new Permission("app.*"); var r = new Permission("app.*"); @@ -46,7 +46,7 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_check_for_equal_permissions() + public void Should_check_when_equal_permissions() { var g = new Permission("app.contents"); var r = new Permission("app.contents"); @@ -57,18 +57,18 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_check_for_given_parent_of_requested() + public void Should_check_when_given_is_parent_of_requested() { var g = new Permission("app"); var r = new Permission("app.contents"); Assert.True(g.Allows(r)); - Assert.False(g.Includes(r)); + Assert.True(g.Includes(r)); } [Fact] - public void Should_check_for_requested_parent_of_given() + public void Should_check_when_requested_is_parent_of_given() { var g = new Permission("app.contents"); var r = new Permission("app"); @@ -79,7 +79,7 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_check_for_given_wildcard_of_requested() + public void Should_check_when_given_is_wildcard_of_requested() { var g = new Permission("app.*"); var r = new Permission("app.contents"); @@ -90,7 +90,7 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_check_for_requested_wildcard_of_given() + public void Should_check_when_requested_is_wildcard_of_given() { var g = new Permission("app.contents"); var r = new Permission("app.*"); @@ -101,7 +101,7 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_check_for_given_has_alternatives_of_requested() + public void Should_check_when_given_is_has_alternatives_of_requested() { var g = new Permission("app.contents|schemas"); var r = new Permission("app.contents"); @@ -112,7 +112,7 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_check_for_requested_has_alternatives_of_given() + public void Should_check_when_requested_is_has_alternatives_of_given() { var g = new Permission("app.contents"); var r = new Permission("app.contents|schemas"); @@ -123,18 +123,7 @@ namespace Squidex.Infrastructure.Security } [Fact] - public void Should_return_false_if_given_permission_has_no_wildcard_but_requested_has() - { - var g = new Permission("app.contents"); - var r = new Permission("app.*"); - - Assert.False(g.Allows(r)); - - Assert.True(g.Includes(r)); - } - - [Fact] - public void Should_check_for_requested_is_null() + public void Should_check_when_requested_is_null() { var g = new Permission("app.contents");