Skip to content

Commit

Permalink
Merge branch 'SPSH-848-person-permissions' into SPSH-852-performance
Browse files Browse the repository at this point in the history
  • Loading branch information
@casparneumann-cap
casparneumann-cap committed Jul 15, 2024
2 parents e05936b + 31cc4a4 commit 320200a
Show file tree
Hide file tree
Showing 9 changed files with 91 additions and 63 deletions.
52 changes: 15 additions & 37 deletions src/modules/authentication/domain/person-permissions.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ describe('PersonPermissions', () => {
});
});

describe('hasSystemrechtAtOrganisation', () => {
describe('hasSystemrechteAtOrganisation', () => {
it('should return true if person has the recht', async () => {
const person: Person<true> = Person.construct(
faker.string.uuid(),
Expand All @@ -255,33 +255,25 @@ describe('PersonPermissions', () => {
undefined,
faker.string.uuid(),
);
const personenkontexte: Personenkontext<true>[] = [
personenkontextFactory.construct('1', faker.date.past(), faker.date.recent(), '1', '1', '1'),
];
dbiamPersonenkontextRepoMock.findByPerson.mockResolvedValueOnce(personenkontexte);
rolleRepoMock.findByIds.mockResolvedValueOnce(
new Map([['1', createMock<Rolle<true>>({ hasSystemRecht: () => true })]]),
);
organisationRepoMock.findChildOrgasForIds.mockResolvedValueOnce([
createMock<OrganisationDo<true>>({ id: '2' }),
]);
dbiamPersonenkontextRepoMock.hasSystemrechtAtOrganisation.mockResolvedValueOnce(true);

const personPermissions: PersonPermissions = new PersonPermissions(
dbiamPersonenkontextRepoMock,
organisationRepoMock,
rolleRepoMock,
person,
);
const result: boolean = await personPermissions.hasSystemrechtAtOrganisation('2', [
const result: boolean = await personPermissions.hasSystemrechteAtOrganisation('2', [
RollenSystemRecht.PERSONEN_VERWALTEN,
]);

expect(result).toBe(true);
expect(result).toBeTruthy();
expect(dbiamPersonenkontextRepoMock.hasSystemrechtAtOrganisation).toHaveBeenCalledTimes(1);
});
});

describe('hasSystemrechtAtOrganisation', () => {
it('should return true if person has the recht at the root', async () => {
describe('hasSystemrechteAtRootOrganisation', () => {
it('should return true if person has the recht', async () => {
const person: Person<true> = Person.construct(
faker.string.uuid(),
faker.date.past(),
Expand All @@ -293,35 +285,21 @@ describe('PersonPermissions', () => {
undefined,
faker.string.uuid(),
);
const personenkontexte: Personenkontext<true>[] = [
personenkontextFactory.construct(
'1',
faker.date.past(),
faker.date.recent(),
'1',
organisationRepoMock.ROOT_ORGANISATION_ID,
'1',
),
];
dbiamPersonenkontextRepoMock.findByPerson.mockResolvedValueOnce(personenkontexte);
rolleRepoMock.findByIds.mockResolvedValueOnce(
new Map([['1', createMock<Rolle<true>>({ hasSystemRecht: () => true })]]),
);
organisationRepoMock.findChildOrgasForIds.mockResolvedValueOnce([
createMock<OrganisationDo<true>>({ id: organisationRepoMock.ROOT_ORGANISATION_ID }),
]);
dbiamPersonenkontextRepoMock.hasSystemrechtAtOrganisation.mockResolvedValue(true);

const personPermissions: PersonPermissions = new PersonPermissions(
dbiamPersonenkontextRepoMock,
organisationRepoMock,
rolleRepoMock,
person,
);
const result: boolean = await personPermissions.hasSystemrechtAtRootOrganisation([
const result: boolean = await personPermissions.hasSystemrechteAtRootOrganisation([
RollenSystemRecht.PERSONEN_VERWALTEN,
RollenSystemRecht.KLASSEN_VERWALTEN,
]);

expect(result).toBe(true);
expect(result).toBeTruthy();
expect(dbiamPersonenkontextRepoMock.hasSystemrechtAtOrganisation).toHaveBeenCalledTimes(2);
});
});

Expand All @@ -344,7 +322,7 @@ describe('PersonPermissions', () => {
rolleRepoMock,
person,
);
jest.spyOn(personPermissions, 'hasSystemrechtAtRootOrganisation').mockResolvedValueOnce(true);
jest.spyOn(personPermissions, 'hasSystemrechteAtRootOrganisation').mockResolvedValueOnce(true);

const result: boolean = await personPermissions.canModifyPerson('2');

Expand Down Expand Up @@ -373,7 +351,7 @@ describe('PersonPermissions', () => {
rolleRepoMock,
person,
);
jest.spyOn(personPermissions, 'hasSystemrechtAtRootOrganisation').mockResolvedValueOnce(false);
jest.spyOn(personPermissions, 'hasSystemrechteAtRootOrganisation').mockResolvedValueOnce(false);

const result: boolean = await personPermissions.canModifyPerson('2');

Expand Down Expand Up @@ -402,7 +380,7 @@ describe('PersonPermissions', () => {
rolleRepoMock,
person,
);
jest.spyOn(personPermissions, 'hasSystemrechtAtRootOrganisation').mockResolvedValueOnce(false);
jest.spyOn(personPermissions, 'hasSystemrechteAtRootOrganisation').mockResolvedValueOnce(false);

const result: boolean = await personPermissions.canModifyPerson('2');

Expand Down
30 changes: 23 additions & 7 deletions src/modules/authentication/domain/person-permissions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ export class PersonPermissions {
});
}

/**
* @deprecated Inefficient
*/
public async getOrgIdsWithSystemrecht(
systemrechte: RollenSystemRecht[],
withChildren: boolean = false,
Expand Down Expand Up @@ -91,24 +94,37 @@ export class PersonPermissions {
return Array.from(organisationIDs);
}

public async hasSystemrechtAtOrganisation(
public async hasSystemrechteAtOrganisation(
organisationId: OrganisationID,
systemrechte: RollenSystemRecht[],
): Promise<boolean> {
const orgsWithRecht: OrganisationID[] = await this.getOrgIdsWithSystemrecht(systemrechte, true);
const checks: Promise<boolean>[] = systemrechte.map(
(systemrecht: RollenSystemRecht): Promise<boolean> =>
this.hasSystemrechtAtOrganisation(organisationId, systemrecht),
);
const results: boolean[] = await Promise.all(checks);

return orgsWithRecht.includes(organisationId);
return results.every((result: boolean): boolean => result);
}

public async hasSystemrechtAtRootOrganisation(systemrechte: RollenSystemRecht[]): Promise<boolean> {
const orgsWithRecht: OrganisationID[] = await this.getOrgIdsWithSystemrecht(systemrechte, true);
public async hasSystemrechteAtRootOrganisation(systemrechte: RollenSystemRecht[]): Promise<boolean> {
return this.hasSystemrechteAtOrganisation(this.organisationRepo.ROOT_ORGANISATION_ID, systemrechte);
}

return orgsWithRecht.includes(this.organisationRepo.ROOT_ORGANISATION_ID);
public async hasSystemrechtAtOrganisation(
organisationId: OrganisationID,
systemrecht: RollenSystemRecht,
): Promise<boolean> {
return this.personenkontextRepo.hasSystemrechtAtOrganisation(
this.cachedPersonFields.id,
organisationId,
systemrecht,
);
}

public async canModifyPerson(personId: PersonID): Promise<boolean> {
{
const hasModifyRechtAtRoot: boolean = await this.hasSystemrechtAtRootOrganisation([
const hasModifyRechtAtRoot: boolean = await this.hasSystemrechteAtRootOrganisation([
RollenSystemRecht.PERSONEN_VERWALTEN,
]);

Expand Down
8 changes: 4 additions & 4 deletions ...modules/personenkontext/api/dbiam-personenkontext-workflow.controller.integration-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ describe('DbiamPersonenkontextWorkflowController Integration Test', () => {
);

const personpermissions: DeepMocked<PersonPermissions> = createMock();
personpermissions.hasSystemrechtAtOrganisation.mockResolvedValueOnce(true);
personpermissions.hasSystemrechtAtOrganisation.mockResolvedValue(true);
personpermissionsRepoMock.loadPersonPermissions.mockResolvedValue(personpermissions);

const response: Response = await request(app.getHttpServer() as App)
Expand All @@ -195,7 +195,7 @@ describe('DbiamPersonenkontextWorkflowController Integration Test', () => {
}),
);
const permissions: DeepMocked<PersonPermissions> = createMock<PersonPermissions>();
permissions.hasSystemrechtAtOrganisation.mockResolvedValueOnce(true);
permissions.hasSystemrechtAtOrganisation.mockResolvedValue(true);
permissions.canModifyPerson.mockResolvedValueOnce(true);
personpermissionsRepoMock.loadPersonPermissions.mockResolvedValue(permissions);

Expand Down Expand Up @@ -223,7 +223,7 @@ describe('DbiamPersonenkontextWorkflowController Integration Test', () => {
);

const personpermissions: DeepMocked<PersonPermissions> = createMock();
personpermissions.hasSystemrechtAtOrganisation.mockResolvedValueOnce(true);
personpermissions.hasSystemrechtAtOrganisation.mockResolvedValue(true);
personpermissionsRepoMock.loadPersonPermissions.mockResolvedValue(personpermissions);

const response: Response = await request(app.getHttpServer() as App)
Expand Down Expand Up @@ -254,7 +254,7 @@ describe('DbiamPersonenkontextWorkflowController Integration Test', () => {
);

const personpermissions: DeepMocked<PersonPermissions> = createMock();
personpermissions.hasSystemrechtAtOrganisation.mockResolvedValueOnce(false);
personpermissions.hasSystemrechteAtOrganisation.mockResolvedValueOnce(false);
personpermissionsRepoMock.loadPersonPermissions.mockResolvedValue(personpermissions);

const response: Response = await request(app.getHttpServer() as App)
Expand Down
12 changes: 6 additions & 6 deletions src/modules/personenkontext/api/dbiam-personenkontext.controller.integration.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ describe('dbiam Personenkontext API', () => {
const personpermissions: DeepMocked<PersonPermissions> = createMock();
personpermissionsRepoMock.loadPersonPermissions.mockResolvedValue(personpermissions);
personpermissions.getOrgIdsWithSystemrecht.mockResolvedValueOnce([]);
personpermissions.hasSystemrechtAtRootOrganisation.mockResolvedValueOnce(false);
personpermissions.hasSystemrechteAtRootOrganisation.mockResolvedValueOnce(false);

const response: Response = await request(app.getHttpServer() as App)
.get(`/dbiam/personenkontext/${faker.string.uuid()}`)
Expand Down Expand Up @@ -221,7 +221,7 @@ describe('dbiam Personenkontext API', () => {
const personpermissions: DeepMocked<PersonPermissions> = createMock();
personpermissionsRepoMock.loadPersonPermissions.mockResolvedValue(personpermissions);
personpermissions.getOrgIdsWithSystemrecht.mockResolvedValueOnce([organisation.id]);
personpermissions.hasSystemrechtAtRootOrganisation.mockResolvedValueOnce(true);
personpermissions.hasSystemrechteAtRootOrganisation.mockResolvedValueOnce(true);

const response: Response = await request(app.getHttpServer() as App)
.post('/dbiam/personenkontext')
Expand Down Expand Up @@ -253,7 +253,7 @@ describe('dbiam Personenkontext API', () => {
const personpermissions: DeepMocked<PersonPermissions> = createMock();
personpermissionsRepoMock.loadPersonPermissions.mockResolvedValue(personpermissions);
personpermissions.getOrgIdsWithSystemrecht.mockResolvedValueOnce([schule.id, klasse.id]);
personpermissions.hasSystemrechtAtRootOrganisation.mockResolvedValueOnce(true);
personpermissions.hasSystemrechteAtRootOrganisation.mockResolvedValueOnce(true);

const response: Response = await request(app.getHttpServer() as App)
.post('/dbiam/personenkontext')
Expand Down Expand Up @@ -282,7 +282,7 @@ describe('dbiam Personenkontext API', () => {
);
const permissions: DeepMocked<PersonPermissions> = createMock<PersonPermissions>();
personpermissionsRepoMock.loadPersonPermissions.mockResolvedValueOnce(permissions);
permissions.hasSystemrechtAtOrganisation.mockResolvedValueOnce(true);
permissions.hasSystemrechteAtOrganisation.mockResolvedValueOnce(true);
permissions.canModifyPerson.mockResolvedValueOnce(true);

const response: Response = await request(app.getHttpServer() as App)
Expand Down Expand Up @@ -408,7 +408,7 @@ describe('dbiam Personenkontext API', () => {

const personpermissions: DeepMocked<PersonPermissions> = createMock();
personpermissionsRepoMock.loadPersonPermissions.mockResolvedValue(personpermissions);
personpermissions.hasSystemrechtAtOrganisation.mockResolvedValueOnce(false);
personpermissions.hasSystemrechteAtOrganisation.mockResolvedValueOnce(false);

const response: Response = await request(app.getHttpServer() as App)
.post('/dbiam/personenkontext')
Expand Down Expand Up @@ -439,7 +439,7 @@ describe('dbiam Personenkontext API', () => {

const personpermissions: DeepMocked<PersonPermissions> = createMock();
personpermissionsRepoMock.loadPersonPermissions.mockResolvedValue(personpermissions);
personpermissions.hasSystemrechtAtOrganisation.mockResolvedValueOnce(false);
personpermissions.hasSystemrechteAtOrganisation.mockResolvedValueOnce(false);

const response: Response = await request(app.getHttpServer() as App)
.post('/dbiam/personenkontext')
Expand Down
4 changes: 2 additions & 2 deletions src/modules/personenkontext/domain/personenkontext-creation.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ describe('PersonenkontextCreationService', () => {
organisationRepositoryMock.findById.mockResolvedValueOnce(
createMock<Organisation<true>>({ typ: OrganisationsTyp.LAND }),
);
personpermissionsMock.hasSystemrechtAtOrganisation.mockResolvedValueOnce(false);
personpermissionsMock.hasSystemrechteAtOrganisation.mockResolvedValueOnce(false);

const result: PersonPersonenkontext | DomainError = await sut.createPersonWithPersonenkontext(
personpermissionsMock,
Expand All @@ -215,7 +215,7 @@ describe('PersonenkontextCreationService', () => {
organisationRepositoryMock.findById.mockResolvedValueOnce(
createMock<Organisation<true>>({ typ: OrganisationsTyp.LAND }),
);
personpermissionsMock.hasSystemrechtAtOrganisation.mockResolvedValueOnce(true);
personpermissionsMock.hasSystemrechteAtOrganisation.mockResolvedValueOnce(true);
personRepositoryMock.create.mockResolvedValueOnce(
new KeycloakClientError('Username or email already exists'),
);
Expand Down
2 changes: 1 addition & 1 deletion src/modules/personenkontext/domain/personenkontext-workflow.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ export class PersonenkontextWorkflowAggregate {
organisationId: string,
): Promise<Option<DomainError>> {
// Check if logged in person has permission
const hasPermissionAtOrga: boolean = await permissions.hasSystemrechtAtOrganisation(organisationId, [
const hasPermissionAtOrga: boolean = await permissions.hasSystemrechteAtOrganisation(organisationId, [
RollenSystemRecht.PERSONEN_VERWALTEN,
]);

Expand Down
8 changes: 4 additions & 4 deletions src/modules/personenkontext/domain/personenkontext.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ describe('Personenkontext aggregate', () => {
describe('checkPermissions', () => {
it('should return MissingPermissionsError, if logged in user is not authorized at organisation', async () => {
const permissions: DeepMocked<PersonPermissions> = createMock<PersonPermissions>();
permissions.hasSystemrechtAtOrganisation.mockResolvedValueOnce(false); // Check orga permissions
permissions.hasSystemrechteAtOrganisation.mockResolvedValueOnce(false); // Check orga permissions

const personenkontext: Personenkontext<false> = personenkontextFactory.createNew(
faker.string.uuid(),
Expand All @@ -188,14 +188,14 @@ describe('Personenkontext aggregate', () => {
new MissingPermissionsError('Unauthorized to manage persons at the organisation'),
);

expect(permissions.hasSystemrechtAtOrganisation).toHaveBeenCalledWith(personenkontext.organisationId, [
expect(permissions.hasSystemrechteAtOrganisation).toHaveBeenCalledWith(personenkontext.organisationId, [
RollenSystemRecht.PERSONEN_VERWALTEN,
]);
});

it('should return MissingPermissionsError, if target person can not be modified by logged in user', async () => {
const permissions: DeepMocked<PersonPermissions> = createMock<PersonPermissions>();
permissions.hasSystemrechtAtOrganisation.mockResolvedValueOnce(true); // Check orga permissions
permissions.hasSystemrechteAtOrganisation.mockResolvedValueOnce(true); // Check orga permissions
const rolleMock: DeepMocked<Rolle<true>> = createMock<Rolle<true>>();
rolleMock.canBeAssignedToOrga.mockResolvedValueOnce(true); // Check rolle<->orga validity
rolleRepoMock.findById.mockResolvedValueOnce(rolleMock);
Expand All @@ -214,7 +214,7 @@ describe('Personenkontext aggregate', () => {

it('should not return an error, if kontext is valid', async () => {
const permissions: DeepMocked<PersonPermissions> = createMock<PersonPermissions>();
permissions.hasSystemrechtAtOrganisation.mockResolvedValueOnce(true); // Check orga permissions
permissions.hasSystemrechteAtOrganisation.mockResolvedValueOnce(true); // Check orga permissions
const rolleMock: DeepMocked<Rolle<true>> = createMock<Rolle<true>>();
rolleMock.canBeAssignedToOrga.mockResolvedValueOnce(true); // Check rolle<->orga validity
rolleRepoMock.findById.mockResolvedValueOnce(rolleMock);
Expand Down
2 changes: 1 addition & 1 deletion src/modules/personenkontext/domain/personenkontext.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ export class Personenkontext<WasPersisted extends boolean> {
public async checkPermissions(permissions: PersonPermissions): Promise<Option<DomainError>> {
// Check if logged in person has permission
{
const hasPermissionAtOrga: boolean = await permissions.hasSystemrechtAtOrganisation(this.organisationId, [
const hasPermissionAtOrga: boolean = await permissions.hasSystemrechteAtOrganisation(this.organisationId, [
RollenSystemRecht.PERSONEN_VERWALTEN,
]);

Expand Down
Loading

0 comments on commit 320200a

Please sign in to comment.