Skip to content

Commit b9a1045

Browse files
tdonohuetdonohue
authored
Merge pull request DSpace#3049 from atmire/w2p-114858_refactor_guards_as_functions-8.0
Fix: Item edit pages not hidden/redirect for anon or non-authorised users
2 parents ebf9469 + 553bada commit b9a1045

62 files changed

Lines changed: 1281 additions & 1284 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎src/app/access-control/access-control-routes.ts‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,13 @@
11
import { AbstractControl } from '@angular/forms';
2-
import {
3-
mapToCanActivate,
4-
Route,
5-
} from '@angular/router';
2+
import { Route } from '@angular/router';
63
import {
74
DYNAMIC_ERROR_MESSAGES_MATCHER,
85
DynamicErrorMessagesMatcher,
96
} from '@ng-dynamic-forms/core';
107

118
import { i18nBreadcrumbResolver } from '../core/breadcrumbs/i18n-breadcrumb.resolver';
12-
import { GroupAdministratorGuard } from '../core/data/feature-authorization/feature-authorization-guard/group-administrator.guard';
13-
import { SiteAdministratorGuard } from '../core/data/feature-authorization/feature-authorization-guard/site-administrator.guard';
9+
import { groupAdministratorGuard } from '../core/data/feature-authorization/feature-authorization-guard/group-administrator.guard';
10+
import { siteAdministratorGuard } from '../core/data/feature-authorization/feature-authorization-guard/site-administrator.guard';
1411
import {
1512
EPERSON_PATH,
1613
GROUP_PATH,
@@ -20,15 +17,15 @@ import { EPeopleRegistryComponent } from './epeople-registry/epeople-registry.co
2017
import { EPersonFormComponent } from './epeople-registry/eperson-form/eperson-form.component';
2118
import { EPersonResolver } from './epeople-registry/eperson-resolver.service';
2219
import { GroupFormComponent } from './group-registry/group-form/group-form.component';
23-
import { GroupPageGuard } from './group-registry/group-page.guard';
20+
import { groupPageGuard } from './group-registry/group-page.guard';
2421
import { GroupsRegistryComponent } from './group-registry/groups-registry.component';
2522

2623
/**
2724
* Condition for displaying error messages on email form field
2825
*/
2926
export const ValidateEmailErrorStateMatcher: DynamicErrorMessagesMatcher =
3027
(control: AbstractControl, model: any, hasFocus: boolean) => {
31-
return (control.touched && !hasFocus) || (control.errors?.emailTaken && hasFocus);
28+
return ( control.touched && !hasFocus ) || ( control.errors?.emailTaken && hasFocus );
3229
};
3330

3431
const providers = [
@@ -46,7 +43,7 @@ export const ROUTES: Route[] = [
4643
},
4744
providers,
4845
data: { title: 'admin.access-control.epeople.title', breadcrumbKey: 'admin.access-control.epeople' },
49-
canActivate: mapToCanActivate([SiteAdministratorGuard]),
46+
canActivate: [siteAdministratorGuard],
5047
},
5148
{
5249
path: `${EPERSON_PATH}/create`,
@@ -56,7 +53,7 @@ export const ROUTES: Route[] = [
5653
},
5754
providers,
5855
data: { title: 'admin.access-control.epeople.add.title', breadcrumbKey: 'admin.access-control.epeople.add' },
59-
canActivate: mapToCanActivate([SiteAdministratorGuard]),
56+
canActivate: [siteAdministratorGuard],
6057
},
6158
{
6259
path: `${EPERSON_PATH}/:id/edit`,
@@ -67,7 +64,7 @@ export const ROUTES: Route[] = [
6764
},
6865
providers,
6966
data: { title: 'admin.access-control.epeople.edit.title', breadcrumbKey: 'admin.access-control.epeople.edit' },
70-
canActivate: mapToCanActivate([SiteAdministratorGuard]),
67+
canActivate: [siteAdministratorGuard],
7168
},
7269
{
7370
path: GROUP_PATH,
@@ -77,7 +74,7 @@ export const ROUTES: Route[] = [
7774
},
7875
providers,
7976
data: { title: 'admin.access-control.groups.title', breadcrumbKey: 'admin.access-control.groups' },
80-
canActivate: mapToCanActivate([GroupAdministratorGuard]),
77+
canActivate: [groupAdministratorGuard],
8178
},
8279
{
8380
path: `${GROUP_PATH}/create`,
@@ -90,7 +87,7 @@ export const ROUTES: Route[] = [
9087
title: 'admin.access-control.groups.title.addGroup',
9188
breadcrumbKey: 'admin.access-control.groups.addGroup',
9289
},
93-
canActivate: mapToCanActivate([GroupAdministratorGuard]),
90+
canActivate: [groupAdministratorGuard],
9491
},
9592
{
9693
path: `${GROUP_PATH}/:groupId/edit`,
@@ -103,7 +100,7 @@ export const ROUTES: Route[] = [
103100
title: 'admin.access-control.groups.title.singleGroup',
104101
breadcrumbKey: 'admin.access-control.groups.singleGroup',
105102
},
106-
canActivate: mapToCanActivate([GroupPageGuard]),
103+
canActivate: [groupPageGuard],
107104
},
108105
{
109106
path: 'bulk-access',
@@ -112,6 +109,6 @@ export const ROUTES: Route[] = [
112109
breadcrumb: i18nBreadcrumbResolver,
113110
},
114111
data: { title: 'admin.access-control.bulk-access.title', breadcrumbKey: 'admin.access-control.bulk-access' },
115-
canActivate: mapToCanActivate([SiteAdministratorGuard]),
112+
canActivate: [siteAdministratorGuard],
116113
},
117114
];

‎src/app/access-control/group-registry/group-page.guard.spec.ts‎

Lines changed: 42 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,24 @@
1+
import {
2+
TestBed,
3+
waitForAsync,
4+
} from '@angular/core/testing';
15
import {
26
ActivatedRouteSnapshot,
37
Router,
8+
UrlTree,
49
} from '@angular/router';
5-
import { of as observableOf } from 'rxjs';
10+
import {
11+
Observable,
12+
of as observableOf,
13+
} from 'rxjs';
614

715
import { AuthService } from '../../core/auth/auth.service';
816
import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
917
import { FeatureID } from '../../core/data/feature-authorization/feature-id';
1018
import { HALEndpointService } from '../../core/shared/hal-endpoint.service';
11-
import { GroupPageGuard } from './group-page.guard';
19+
import { groupPageGuard } from './group-page.guard';
20+
21+
jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000; // Increase timeout to 10 seconds
1222

1323
describe('GroupPageGuard', () => {
1424
const groupsEndpointUrl = 'https://test.org/api/eperson/groups';
@@ -20,42 +30,54 @@ describe('GroupPageGuard', () => {
2030
},
2131
} as unknown as ActivatedRouteSnapshot;
2232

23-
let guard: GroupPageGuard;
2433
let halEndpointService: HALEndpointService;
2534
let authorizationService: AuthorizationDataService;
2635
let router: Router;
2736
let authService: AuthService;
2837

29-
beforeEach(() => {
38+
function init() {
3039
halEndpointService = jasmine.createSpyObj(['getEndpoint']);
31-
(halEndpointService as any).getEndpoint.and.returnValue(observableOf(groupsEndpointUrl));
40+
( halEndpointService as any ).getEndpoint.and.returnValue(observableOf(groupsEndpointUrl));
3241

3342
authorizationService = jasmine.createSpyObj(['isAuthorized']);
3443
// NOTE: value is set in beforeEach
3544

3645
router = jasmine.createSpyObj(['parseUrl']);
37-
(router as any).parseUrl.and.returnValue = {};
46+
( router as any ).parseUrl.and.returnValue = {};
3847

3948
authService = jasmine.createSpyObj(['isAuthenticated']);
40-
(authService as any).isAuthenticated.and.returnValue(observableOf(true));
49+
( authService as any ).isAuthenticated.and.returnValue(observableOf(true));
4150

42-
guard = new GroupPageGuard(halEndpointService, authorizationService, router, authService);
43-
});
51+
TestBed.configureTestingModule({
52+
providers: [
53+
{ provide: AuthorizationDataService, useValue: authorizationService },
54+
{ provide: Router, useValue: router },
55+
{ provide: AuthService, useValue: authService },
56+
{ provide: HALEndpointService, useValue: halEndpointService },
57+
],
58+
});
59+
}
60+
61+
beforeEach(waitForAsync(() => {
62+
init();
63+
}));
4464

4565
it('should be created', () => {
46-
expect(guard).toBeTruthy();
66+
expect(groupPageGuard).toBeTruthy();
4767
});
4868

4969
describe('canActivate', () => {
5070
describe('when the current user can manage the group', () => {
5171
beforeEach(() => {
52-
(authorizationService as any).isAuthorized.and.returnValue(observableOf(true));
72+
( authorizationService as any ).isAuthorized.and.returnValue(observableOf(true));
5373
});
5474

5575
it('should return true', (done) => {
56-
guard.canActivate(
57-
routeSnapshotWithGroupId, { url: 'current-url' } as any,
58-
).subscribe((result) => {
76+
const result$ = TestBed.runInInjectionContext(() => {
77+
return groupPageGuard()(routeSnapshotWithGroupId, { url: 'current-url' } as any);
78+
}) as Observable<boolean | UrlTree>;
79+
80+
result$.subscribe((result) => {
5981
expect(authorizationService.isAuthorized).toHaveBeenCalledWith(
6082
FeatureID.CanManageGroup, groupEndpointUrl, undefined,
6183
);
@@ -71,15 +93,18 @@ describe('GroupPageGuard', () => {
7193
});
7294

7395
it('should not return true', (done) => {
74-
guard.canActivate(
75-
routeSnapshotWithGroupId, { url: 'current-url' } as any,
76-
).subscribe((result) => {
96+
const result$ = TestBed.runInInjectionContext(() => {
97+
return groupPageGuard()(routeSnapshotWithGroupId, { url: 'current-url' } as any);
98+
}) as Observable<boolean | UrlTree>;
99+
100+
result$.subscribe((result) => {
77101
expect(authorizationService.isAuthorized).toHaveBeenCalledWith(
78102
FeatureID.CanManageGroup, groupEndpointUrl, undefined,
79103
);
80104
expect(result).not.toBeTrue();
81105
done();
82106
});
107+
83108
});
84109
});
85110
});

‎src/app/access-control/group-registry/group-page.guard.ts‎

Lines changed: 23 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
import { Injectable } from '@angular/core';
1+
import { inject } from '@angular/core';
22
import {
33
ActivatedRouteSnapshot,
4-
Router,
4+
CanActivateFn,
55
RouterStateSnapshot,
66
} from '@angular/router';
77
import {
@@ -10,34 +10,29 @@ import {
1010
} from 'rxjs';
1111
import { map } from 'rxjs/operators';
1212

13-
import { AuthService } from '../../core/auth/auth.service';
14-
import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
15-
import { SomeFeatureAuthorizationGuard } from '../../core/data/feature-authorization/feature-authorization-guard/some-feature-authorization.guard';
13+
import {
14+
someFeatureAuthorizationGuard,
15+
StringGuardParamFn,
16+
} from '../../core/data/feature-authorization/feature-authorization-guard/some-feature-authorization.guard';
1617
import { FeatureID } from '../../core/data/feature-authorization/feature-id';
1718
import { HALEndpointService } from '../../core/shared/hal-endpoint.service';
1819

19-
@Injectable({
20-
providedIn: 'root',
21-
})
22-
export class GroupPageGuard extends SomeFeatureAuthorizationGuard {
23-
24-
protected groupsEndpoint = 'groups';
25-
26-
constructor(protected halEndpointService: HALEndpointService,
27-
protected authorizationService: AuthorizationDataService,
28-
protected router: Router,
29-
protected authService: AuthService) {
30-
super(authorizationService, router, authService);
31-
}
32-
33-
getFeatureIDs(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<FeatureID[]> {
34-
return observableOf([FeatureID.CanManageGroup]);
35-
}
20+
const defaultGroupPageGetObjectUrl: StringGuardParamFn = (
21+
route: ActivatedRouteSnapshot,
22+
state: RouterStateSnapshot,
23+
): Observable<string> => {
24+
const halEndpointService = inject(HALEndpointService);
25+
const groupsEndpoint = 'groups';
3626

37-
getObjectUrl(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<string> {
38-
return this.halEndpointService.getEndpoint(this.groupsEndpoint).pipe(
39-
map(groupsUrl => `${groupsUrl}/${route?.params?.groupId}`),
40-
);
41-
}
27+
return halEndpointService.getEndpoint(groupsEndpoint).pipe(
28+
map(groupsUrl => `${groupsUrl}/${route?.params?.groupId}`),
29+
);
30+
};
4231

43-
}
32+
export const groupPageGuard = (
33+
getObjectUrl = defaultGroupPageGetObjectUrl,
34+
getEPersonUuid?: StringGuardParamFn,
35+
): CanActivateFn => someFeatureAuthorizationGuard(
36+
() => observableOf([FeatureID.CanManageGroup]),
37+
getObjectUrl,
38+
getEPersonUuid);

‎src/app/admin/admin-notify-dashboard/admin-notify-dashboard-routes.ts‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,15 @@
1-
import {
2-
mapToCanActivate,
3-
Route,
4-
} from '@angular/router';
1+
import { Route } from '@angular/router';
52

63
import { i18nBreadcrumbResolver } from '../../core/breadcrumbs/i18n-breadcrumb.resolver';
74
import { notifyInfoGuard } from '../../core/coar-notify/notify-info/notify-info.guard';
8-
import { SiteAdministratorGuard } from '../../core/data/feature-authorization/feature-authorization-guard/site-administrator.guard';
5+
import { siteAdministratorGuard } from '../../core/data/feature-authorization/feature-authorization-guard/site-administrator.guard';
96
import { AdminNotifyDashboardComponent } from './admin-notify-dashboard.component';
107
import { AdminNotifyIncomingComponent } from './admin-notify-logs/admin-notify-incoming/admin-notify-incoming.component';
118
import { AdminNotifyOutgoingComponent } from './admin-notify-logs/admin-notify-outgoing/admin-notify-outgoing.component';
129

1310
export const ROUTES: Route[] = [
1411
{
15-
canActivate: [...mapToCanActivate([SiteAdministratorGuard]), notifyInfoGuard],
12+
canActivate: [siteAdministratorGuard, notifyInfoGuard],
1613
path: '',
1714
resolve: {
1815
breadcrumb: i18nBreadcrumbResolver,
@@ -30,7 +27,7 @@ export const ROUTES: Route[] = [
3027
breadcrumb: i18nBreadcrumbResolver,
3128
},
3229
component: AdminNotifyIncomingComponent,
33-
canActivate: [...mapToCanActivate([SiteAdministratorGuard]), notifyInfoGuard],
30+
canActivate: [siteAdministratorGuard, notifyInfoGuard],
3431
data: {
3532
title: 'admin.notify.dashboard.page.title',
3633
breadcrumbKey: 'admin.notify.dashboard',
@@ -42,7 +39,7 @@ export const ROUTES: Route[] = [
4239
breadcrumb: i18nBreadcrumbResolver,
4340
},
4441
component: AdminNotifyOutgoingComponent,
45-
canActivate: [...mapToCanActivate([SiteAdministratorGuard]), notifyInfoGuard],
42+
canActivate: [siteAdministratorGuard, notifyInfoGuard],
4643
data: {
4744
title: 'admin.notify.dashboard.page.title',
4845
breadcrumbKey: 'admin.notify.dashboard',

0 commit comments

Comments
 (0)