Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guards fix #2129

Merged
merged 4 commits into from
Dec 18, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/app/app.routing.ts
Original file line number Diff line number Diff line change
@@ -53,6 +53,7 @@ export const allRoutes: Routes = [
},
{
path: "admin",
canActivate: [AuthGuard],
// add directly without lazy-loading so that Menu can detect permissions for child routes
children: AdminModule.routes,
},
3 changes: 1 addition & 2 deletions src/app/core/admin/admin.routing.ts
Original file line number Diff line number Diff line change
@@ -5,7 +5,6 @@ import { ConfigImportComponent } from "../../features/config-setup/config-import
import { ConflictResolutionListComponent } from "../../features/conflict-resolution/conflict-resolution-list/conflict-resolution-list.component";
import { UserRoleGuard } from "../permissions/permission-guard/user-role.guard";
import { EntityPermissionGuard } from "../permissions/permission-guard/entity-permission.guard";
import { AuthGuard } from "../session/auth.guard";

export const adminRoutes: Routes = [
{
@@ -24,7 +23,7 @@ export const adminRoutes: Routes = [
entity: "Config",
requiredPermissionOperation: "update",
},
canActivate: [AuthGuard, EntityPermissionGuard],
canActivate: [EntityPermissionGuard],
},

{
Original file line number Diff line number Diff line change
@@ -9,7 +9,7 @@ describe("EntityPermissionGuard", () => {
let mockAbility: jasmine.SpyObj<EntityAbility>;

beforeEach(() => {
mockAbility = jasmine.createSpyObj(["can"]);
mockAbility = jasmine.createSpyObj(["can"], { rules: [{}] });

TestBed.configureTestingModule({
imports: [RouterTestingModule],
@@ -25,10 +25,10 @@ describe("EntityPermissionGuard", () => {
expect(guard).toBeTruthy();
});

it("should use ability to check if current user is allowed", () => {
it("should use ability to check if current user is allowed", async () => {
mockAbility.can.and.returnValue(true);

const result = guard.canActivate({
const result = await guard.canActivate({
routeConfig: { path: "url" },
data: {
requiredPermissionOperation: "update",
@@ -40,7 +40,7 @@ describe("EntityPermissionGuard", () => {
expect(mockAbility.can).toHaveBeenCalledWith("update", "TestEntity");
});

it("should navigate to 404 for real navigation requests without permissions", () => {
it("should navigate to 404 for real navigation requests without permissions", async () => {
const router = TestBed.inject(Router);
spyOn(router, "navigate");
const route = new ActivatedRouteSnapshot();
@@ -50,14 +50,14 @@ describe("EntityPermissionGuard", () => {
});
mockAbility.can.and.returnValue(false);

guard.canActivate(route);
await guard.canActivate(route);

expect(router.navigate).toHaveBeenCalledWith(["/404"]);
});

it("should check 'read' permissions if only entity and no operation is set", () => {
it("should check 'read' permissions if only entity and no operation is set", async () => {
mockAbility.can.and.returnValue(true);
const result = guard.canActivate({
const result = await guard.canActivate({
routeConfig: { path: "default-operation-config" },
data: { entity: "Child" },
} as Partial<ActivatedRouteSnapshot> as ActivatedRouteSnapshot);
@@ -66,24 +66,24 @@ describe("EntityPermissionGuard", () => {
expect(mockAbility.can).toHaveBeenCalledWith("read", "Child");
});

it("should return true as default if no entity or operation are configured", () => {
const result = guard.canActivate({
it("should return true as default if no entity or operation are configured", async () => {
const result = await guard.canActivate({
routeConfig: { path: "no-permission-config" },
} as any);

expect(result).toBeTrue();
expect(mockAbility.can).not.toHaveBeenCalled();
});

it("should evaluate correct route permissions on 'pre-check' (checkRoutePermissions)", () => {
it("should evaluate correct route permissions on 'pre-check' (checkRoutePermissions)", async () => {
mockAbility.can.and.returnValue(true);

TestBed.inject(Router).config.push({
path: "check-route/:id",
data: { requiredPermissionOperation: "update", entityType: "Child" },
});

const result = guard.checkRoutePermissions("check-route/1");
const result = await guard.checkRoutePermissions("check-route/1");

expect(result).toBeTrue();
expect(mockAbility.can).toHaveBeenCalledWith("update", "Child");
Original file line number Diff line number Diff line change
@@ -14,9 +14,9 @@ export class EntityPermissionGuard implements CanActivate {
private ability: EntityAbility,
) {}

canActivate(route: ActivatedRouteSnapshot): boolean {
async canActivate(route: ActivatedRouteSnapshot): Promise<boolean> {
const routeData: RouteData = route.data;
if (this.canAccessRoute(routeData)) {
if (await this.canAccessRoute(routeData)) {
return true;
} else {
if (route instanceof ActivatedRouteSnapshot) {
@@ -27,7 +27,7 @@ export class EntityPermissionGuard implements CanActivate {
}
}

private canAccessRoute(routeData: RouteData) {
private async canAccessRoute(routeData: RouteData) {
const operation = routeData?.["requiredPermissionOperation"] ?? "read";
const primaryEntity =
routeData?.["entityType"] ??
@@ -40,6 +40,11 @@ export class EntityPermissionGuard implements CanActivate {
return true;
}

if (this.ability.rules.length === 0) {
// wait till rules are initialised
await new Promise((res) => this.ability.on("updated", res));
}

return this.ability.can(operation, primaryEntity);
}

64 changes: 50 additions & 14 deletions src/app/core/ui/navigation/navigation/navigation.component.spec.ts
Original file line number Diff line number Diff line change
@@ -15,7 +15,13 @@
* along with ndb-core. If not, see <http://www.gnu.org/licenses/>.
*/

import { ComponentFixture, TestBed, waitForAsync } from "@angular/core/testing";
import {
ComponentFixture,
fakeAsync,
TestBed,
tick,
waitForAsync,
} from "@angular/core/testing";

import { NavigationComponent } from "./navigation.component";
import { MenuItem } from "../menu-item";
@@ -33,7 +39,8 @@ describe("NavigationComponent", () => {

let mockConfigService: jasmine.SpyObj<ConfigService>;
let mockConfigUpdated: BehaviorSubject<Config>;
let mockGuard: jasmine.SpyObj<UserRoleGuard>;
let mockRoleGuard: jasmine.SpyObj<UserRoleGuard>;
let mockEntityGuard: jasmine.SpyObj<EntityPermissionGuard>;

beforeEach(waitForAsync(() => {
mockConfigUpdated = new BehaviorSubject<Config>(null);
@@ -42,14 +49,16 @@ describe("NavigationComponent", () => {
});
mockConfigService.getConfig.and.returnValue({ items: [] });
mockConfigService.getAllConfigs.and.returnValue([]);
mockGuard = jasmine.createSpyObj(["checkRoutePermissions"]);
mockGuard.checkRoutePermissions.and.returnValue(true);
mockRoleGuard = jasmine.createSpyObj(["checkRoutePermissions"]);
mockRoleGuard.checkRoutePermissions.and.returnValue(true);
mockEntityGuard = jasmine.createSpyObj(["checkRoutePermissions"]);
mockEntityGuard.checkRoutePermissions.and.resolveTo(true);

TestBed.configureTestingModule({
imports: [NavigationComponent, MockedTestingModule.withState()],
providers: [
{ provide: UserRoleGuard, useValue: mockGuard },
{ provide: EntityPermissionGuard, useValue: mockGuard },
{ provide: UserRoleGuard, useValue: mockRoleGuard },
{ provide: EntityPermissionGuard, useValue: mockEntityGuard },
{ provide: ConfigService, useValue: mockConfigService },
],
}).compileComponents();
@@ -65,7 +74,7 @@ describe("NavigationComponent", () => {
expect(component).toBeTruthy();
});

it("generates menu items from config", function () {
it("generates menu items from config", fakeAsync(() => {
const testConfig = {
items: [
{ name: "Dashboard", icon: "home", link: "/dashboard" },
@@ -74,23 +83,22 @@ describe("NavigationComponent", () => {
};
mockConfigService.getConfig.and.returnValue(testConfig);
mockConfigUpdated.next(null);
const items = component.menuItems;
tick();

expect(items).toEqual([
expect(component.menuItems).toEqual([
new MenuItem("Dashboard", "home", "/dashboard"),
new MenuItem("Children", "child", "/child"),
]);
});
}));

it("marks items that require admin rights", function () {
it("marks items that require admin rights", fakeAsync(() => {
const testConfig = {
items: [
{ name: "Dashboard", icon: "home", link: "/dashboard" },
{ name: "Children", icon: "child", link: "/child" },
],
};
mockConfigService.getConfig.and.returnValue(testConfig);
mockGuard.checkRoutePermissions.and.callFake((route: string) => {
mockRoleGuard.checkRoutePermissions.and.callFake((route: string) => {
switch (route) {
case "/dashboard":
return false;
@@ -103,11 +111,39 @@ describe("NavigationComponent", () => {

mockConfigService.getConfig.and.returnValue(testConfig);
mockConfigUpdated.next(null);
tick();

expect(component.menuItems).toEqual([
new MenuItem("Children", "child", "/child"),
]);
});
}));

it("should not add menu items if entity permissions are missing", fakeAsync(() => {
const testConfig = {
items: [
{ name: "Dashboard", icon: "home", link: "/dashboard" },
{ name: "Children", icon: "child", link: "/child" },
],
};
mockEntityGuard.checkRoutePermissions.and.callFake((route: string) => {
switch (route) {
case "/dashboard":
return Promise.resolve(false);
case "/child":
return Promise.resolve(true);
default:
return Promise.resolve(false);
}
});

mockConfigService.getConfig.and.returnValue(testConfig);
mockConfigUpdated.next(null);
tick();

expect(component.menuItems).toEqual([
new MenuItem("Children", "child", "/child"),
]);
}));

it("should highlight active menu item", () => {
const routerEvents = TestBed.inject(Router).events as Subject<Event>;
13 changes: 9 additions & 4 deletions src/app/core/ui/navigation/navigation/navigation.component.ts
Original file line number Diff line number Diff line change
@@ -114,12 +114,17 @@ export class NavigationComponent {
/**
* Load menu items from config file
*/
private initMenuItemsFromConfig() {
private async initMenuItemsFromConfig() {
const config: NavigationMenuConfig =
this.configService.getConfig<NavigationMenuConfig>(this.CONFIG_ID);
this.menuItems = config.items
.filter(({ link }) => this.isAccessibleRouteForUser(link))
.map(({ name, icon, link }) => new MenuItem(name, icon, link));
const menuItems = [];
for (const item of config.items) {
if (await this.isAccessibleRouteForUser(item.link)) {
menuItems.push(new MenuItem(item.name, item.icon, item.link));
}
}
this.menuItems = menuItems;
this.activeLink = this.computeActiveLink(location.pathname);
}

private isAccessibleRouteForUser(path: string) {