Skip to content

Commit 67c5083

Browse files
pmachapmanpmachapman
authored
SF-3637 Deduplicate permissions (#3566)
1 parent 1d91620 commit 67c5083

File tree

17 files changed

+264
-142
lines changed

17 files changed

+264
-142
lines changed

‎src/SIL.XForge.Scripture/ClientApp/src/app/core/permissions.service.spec.ts‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -105,16 +105,16 @@ describe('PermissionsService', () => {
105105

106106
const textDoc: Partial<TextDocId> = { projectId: 'project01', bookNum: 41, chapterNum: 1 };
107107

108-
expect(await env.service.canAccessText(cloneDeep(textDoc) as TextDocId)).toBe(true);
108+
expect(await env.service.canAccessTextAsync(cloneDeep(textDoc) as TextDocId)).toBe(true);
109109
}));
110110

111111
it('allows access to text if user is on project and does not have chapter permission', fakeAsync(async () => {
112112
const env = new TestEnvironment();
113-
env.setupProjectData(undefined);
113+
env.setupProjectData(undefined, false);
114114

115115
const textDoc: Partial<TextDocId> = { projectId: 'project01', bookNum: 41, chapterNum: 1 };
116116

117-
expect(await env.service.canAccessText(cloneDeep(textDoc) as TextDocId)).toBe(true);
117+
expect(await env.service.canAccessTextAsync(cloneDeep(textDoc) as TextDocId)).toBe(true);
118118
}));
119119

120120
it('does not allow access to text if user is not on project', fakeAsync(async () => {
@@ -123,7 +123,7 @@ describe('PermissionsService', () => {
123123

124124
const textDoc: Partial<TextDocId> = { projectId: 'project01', bookNum: 41, chapterNum: 1 };
125125

126-
expect(await env.service.canAccessText(cloneDeep(textDoc) as TextDocId)).toBe(false);
126+
expect(await env.service.canAccessTextAsync(cloneDeep(textDoc) as TextDocId)).toBe(false);
127127
}));
128128

129129
it('does not allow access to text if user has no access', fakeAsync(async () => {
@@ -132,7 +132,7 @@ describe('PermissionsService', () => {
132132

133133
const textDoc: Partial<TextDocId> = { projectId: 'project01', bookNum: 41, chapterNum: 1 };
134134

135-
expect(await env.service.canAccessText(cloneDeep(textDoc) as TextDocId)).toBe(false);
135+
expect(await env.service.canAccessTextAsync(cloneDeep(textDoc) as TextDocId)).toBe(false);
136136
}));
137137

138138
it('does not allow access to text if the chapter does not exist', fakeAsync(async () => {
@@ -141,7 +141,7 @@ describe('PermissionsService', () => {
141141

142142
const textDoc: Partial<TextDocId> = { projectId: 'project01', bookNum: 41, chapterNum: 2 };
143143

144-
expect(await env.service.canAccessText(cloneDeep(textDoc) as TextDocId)).toBe(false);
144+
expect(await env.service.canAccessTextAsync(cloneDeep(textDoc) as TextDocId)).toBe(false);
145145
}));
146146

147147
it('checks current user doc to determine if user is on project', fakeAsync(async () => {
@@ -264,7 +264,7 @@ class TestEnvironment {
264264
this.setupUserData();
265265
}
266266

267-
setupProjectData(textPermission?: TextInfoPermission | undefined): void {
267+
setupProjectData(textPermission?: TextInfoPermission | undefined, chapterPermissions: boolean = true): void {
268268
const projectId: string = 'project01';
269269
const permission: TextInfoPermission = textPermission ?? TextInfoPermission.Write;
270270

@@ -284,10 +284,12 @@ class TestEnvironment {
284284
number: 1,
285285
lastVerse: 3,
286286
isValid: true,
287-
permissions: {
288-
user01: permission,
289-
user02: permission
290-
}
287+
permissions: chapterPermissions
288+
? {
289+
user01: permission,
290+
user02: permission
291+
}
292+
: {}
291293
}
292294
],
293295
hasSource: true,

‎src/SIL.XForge.Scripture/ClientApp/src/app/core/permissions.service.ts‎

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import { Injectable } from '@angular/core';
22
import { Operation } from 'realtime-server/lib/esm/common/models/project-rights';
3+
import { SFProjectProfile } from 'realtime-server/lib/esm/scriptureforge/models/sf-project';
34
import { SF_PROJECT_RIGHTS, SFProjectDomain } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-rights';
45
import { isParatextRole, SFProjectRole } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-role';
5-
import { Chapter } from 'realtime-server/lib/esm/scriptureforge/models/text-info';
6+
import { Chapter, TextInfo } from 'realtime-server/lib/esm/scriptureforge/models/text-info';
67
import { TextInfoPermission } from 'realtime-server/lib/esm/scriptureforge/models/text-info-permission';
78
import { UserDoc } from 'xforge-common/models/user-doc';
89
import { UserService } from 'xforge-common/user.service';
@@ -58,25 +59,25 @@ export class PermissionsService {
5859
return isParatextRole(projectDoc.data?.userRoles[currentUserDoc.id] ?? SFProjectRole.None);
5960
}
6061

61-
async canAccessText(textDocId: TextDocId): Promise<boolean> {
62-
// Get the project doc, if the user is on that project
63-
let projectDoc: SFProjectProfileDoc | undefined;
64-
if (textDocId.projectId != null) {
65-
const isUserOnProject = await this.isUserOnProject(textDocId.projectId);
66-
projectDoc = isUserOnProject ? await this.projectService.getProfile(textDocId.projectId) : undefined;
67-
}
68-
62+
/**
63+
* Determines if a user can access the text in the specified project.
64+
* @param textDocId The text document id.
65+
* @param project The project.
66+
* @returns A boolean value.
67+
*/
68+
canAccessText(textDocId: TextDocId, project?: SFProjectProfile): boolean {
6969
// Ensure the user has project level permission to view the text
7070
if (
71-
projectDoc?.data != null &&
72-
SF_PROJECT_RIGHTS.hasRight(projectDoc.data, this.userService.currentUserId, SFProjectDomain.Texts, Operation.View)
71+
project != null &&
72+
SF_PROJECT_RIGHTS.hasRight(project, this.userService.currentUserId, SFProjectDomain.Texts, Operation.View)
7373
) {
7474
// Check chapter permissions
75-
const chapter: Chapter | undefined = projectDoc.data.texts
76-
.find(t => t.bookNum === textDocId.bookNum)
77-
?.chapters.find(c => c.number === textDocId.chapterNum);
78-
if (chapter != null) {
79-
const chapterPermission: string | undefined = chapter.permissions[this.userService.currentUserId];
75+
const text: TextInfo | undefined = project.texts.find(t => t.bookNum === textDocId.bookNum);
76+
const chapter: Chapter | undefined = text?.chapters.find(c => c.number === textDocId.chapterNum);
77+
if (text != null && chapter != null) {
78+
// If the chapter permission is not present, use the book permission instead
79+
const chapterPermission: string | undefined =
80+
chapter.permissions[this.userService.currentUserId] ?? text.permissions[this.userService.currentUserId];
8081
// If there is no chapter permission, they will have access to the chapter as they have access to the project.
8182
// We should only deny access if there is an explicit "None" permission.
8283
return chapterPermission !== TextInfoPermission.None;
@@ -86,6 +87,23 @@ export class PermissionsService {
8687
return false;
8788
}
8889

90+
/**
91+
* Determines if a user can access a text.
92+
* @param textDocId The text document id.
93+
* @returns A promise for a boolean value.
94+
*/
95+
async canAccessTextAsync(textDocId: TextDocId): Promise<boolean> {
96+
// Get the project doc, if the user is on that project
97+
let projectDoc: SFProjectProfileDoc | undefined;
98+
if (textDocId.projectId != null) {
99+
const isUserOnProject = await this.isUserOnProject(textDocId.projectId);
100+
projectDoc = isUserOnProject ? await this.projectService.getProfile(textDocId.projectId) : undefined;
101+
return this.canAccessText(textDocId, projectDoc?.data);
102+
}
103+
104+
return false;
105+
}
106+
89107
canSync(projectDoc: SFProjectProfileDoc, userId?: string): boolean {
90108
if (projectDoc.data == null) {
91109
return false;

‎src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.spec.ts‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -410,18 +410,19 @@ describe('TextDocService', () => {
410410
expect(actual).toBeUndefined();
411411
});
412412

413-
it('should return undefined if the user does not have the permission', () => {
413+
it('should return false if the user does not have the permission', () => {
414414
const env = new TestEnvironment();
415415
const text: Partial<TextInfo> = { chapters: [{ number: 1 } as Chapter] };
416416

417417
// SUT
418418
const actual: boolean | undefined = env.textDocService.hasChapterEditPermissionForText(text as TextInfo, 1);
419-
expect(actual).toBeUndefined();
419+
expect(actual).toBe(false);
420420
});
421421

422-
it('should return false if the user does not have the edit permission', () => {
422+
it('should return false if the user does not have the write permission for the chapter', () => {
423423
const env = new TestEnvironment();
424424
const text: Partial<TextInfo> = {
425+
permissions: { user01: TextInfoPermission.Write },
425426
chapters: [
426427
{ number: 1, permissions: { user01: TextInfoPermission.Read }, lastVerse: 0, isValid: true } as Chapter
427428
]
@@ -432,7 +433,7 @@ describe('TextDocService', () => {
432433
expect(actual).toBe(false);
433434
});
434435

435-
it('should return true if the user has the write permission', () => {
436+
it('should return true if the user has the write permission for the chapter', () => {
436437
const env = new TestEnvironment();
437438
const text: Partial<TextInfo> = {
438439
chapters: [
@@ -444,6 +445,18 @@ describe('TextDocService', () => {
444445
const actual: boolean | undefined = env.textDocService.hasChapterEditPermissionForText(text as TextInfo, 1);
445446
expect(actual).toBe(true);
446447
});
448+
449+
it('should return true if the user has the write permission for the book and no chapter permission set', () => {
450+
const env = new TestEnvironment();
451+
const text: Partial<TextInfo> = {
452+
permissions: { user01: TextInfoPermission.Write },
453+
chapters: [{ number: 1, lastVerse: 0, isValid: true } as Chapter]
454+
};
455+
456+
// SUT
457+
const actual: boolean | undefined = env.textDocService.hasChapterEditPermissionForText(text as TextInfo, 1);
458+
expect(actual).toBe(true);
459+
});
447460
});
448461
});
449462

‎src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,10 +187,12 @@ export class TextDocService {
187187
*/
188188
hasChapterEditPermissionForText(text: TextInfo | undefined, chapterNum: number | undefined): boolean | undefined {
189189
const chapter: Chapter | undefined = text?.chapters.find(c => c.number === chapterNum);
190+
if (chapter == null) return undefined;
190191
// Even though permissions is guaranteed to be there in the model, its not in IndexedDB the first time the project
191192
// is accessed after migration
192-
const permission: string | undefined = chapter?.permissions?.[this.userService.currentUserId];
193-
return permission == null ? undefined : permission === TextInfoPermission.Write;
193+
const permission: string | undefined =
194+
chapter?.permissions?.[this.userService.currentUserId] ?? text?.permissions?.[this.userService.currentUserId];
195+
return permission === TextInfoPermission.Write;
194196
}
195197

196198
/**

‎src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.spec.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ describe('cache service', () => {
2424
describe('load all texts', () => {
2525
it('does not get texts from project service if no permission', fakeAsync(async () => {
2626
const env = new TestEnvironment();
27-
when(mockedPermissionService.canAccessText(anything())).thenResolve(false);
27+
when(mockedPermissionService.canAccessTextAsync(anything())).thenResolve(false);
2828
await env.service.cache(env.projectDoc);
2929
env.wait();
3030

@@ -72,9 +72,9 @@ describe('cache service', () => {
7272

7373
it('gets the source texts if they are present and the user can access', fakeAsync(async () => {
7474
const env = new TestEnvironment();
75-
when(mockedPermissionService.canAccessText(deepEqual(new TextDocId('sourceId', 0, 0, 'target')))).thenResolve(
76-
false
77-
); //remove access for one source doc
75+
when(
76+
mockedPermissionService.canAccessTextAsync(deepEqual(new TextDocId('sourceId', 0, 0, 'target')))
77+
).thenResolve(false); //remove access for one source doc
7878

7979
await env.service.cache(env.projectDoc);
8080
env.wait();
@@ -106,7 +106,7 @@ class TestEnvironment {
106106
});
107107

108108
when(mockedProjectDoc.data).thenReturn(data);
109-
when(mockedPermissionService.canAccessText(anything())).thenResolve(true);
109+
when(mockedPermissionService.canAccessTextAsync(anything())).thenResolve(true);
110110
}
111111

112112
createTexts(): TextInfo[] {

‎src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,13 @@ export class CacheService {
3232
}
3333

3434
const textDocId = new TextDocId(project.id, text.bookNum, chapter.number, 'target');
35-
if (await this.permissionsService.canAccessText(textDocId)) {
35+
if (await this.permissionsService.canAccessTextAsync(textDocId)) {
3636
await this.projectService.getText(textDocId);
3737
}
3838

3939
if (text.hasSource && sourceId != null) {
4040
const sourceTextDocId = new TextDocId(sourceId, text.bookNum, chapter.number, 'target');
41-
if (await this.permissionsService.canAccessText(sourceTextDocId)) {
41+
if (await this.permissionsService.canAccessTextAsync(sourceTextDocId)) {
4242
await this.projectService.getText(sourceTextDocId);
4343
}
4444
}

‎src/SIL.XForge.Scripture/ClientApp/src/app/shared/progress-service/progress.service.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ describe('progress service', () => {
139139

140140
it('cannot train suggestions if no source permission', fakeAsync(async () => {
141141
const env = new TestEnvironment();
142-
when(mockPermissionService.canAccessText(anything())).thenCall((textDocId: TextDocId) => {
142+
when(mockPermissionService.canAccessTextAsync(anything())).thenCall((textDocId: TextDocId) => {
143143
return Promise.resolve(textDocId.projectId !== 'sourceId');
144144
});
145145
tick();
@@ -192,7 +192,7 @@ class TestEnvironment {
192192
when(this.mockProject.id).thenReturn('project01');
193193
when(mockProjectService.changes$).thenReturn(this.project$);
194194

195-
when(mockPermissionService.canAccessText(anything())).thenResolve(true);
195+
when(mockPermissionService.canAccessTextAsync(anything())).thenResolve(true);
196196
when(mockSFProjectService.getProfile('project01')).thenResolve({
197197
data,
198198
id: 'project01',

‎src/SIL.XForge.Scripture/ClientApp/src/app/shared/progress-service/progress.service.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ export class ProgressService extends DataLoadingComponent implements OnDestroy {
304304

305305
// Only retrieve the source text if the user has permission
306306
let sourceNonEmptyVerses: string[] = [];
307-
if (await this.permissionsService.canAccessText(sourceTextDocId)) {
307+
if (await this.permissionsService.canAccessTextAsync(sourceTextDocId)) {
308308
const sourceChapterText: TextDoc = await this.projectService.getText(sourceTextDocId);
309309
sourceNonEmptyVerses = sourceChapterText.getNonEmptyVerses();
310310
}

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.spec.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4967,6 +4967,7 @@ class TestEnvironment {
49674967
when(mockedDraftGenerationService.getLastCompletedBuild(anything())).thenReturn(of({} as BuildDto));
49684968
when(mockedDraftOptionsService.areFormattingOptionsAvailableButUnselected(anything())).thenReturn(false);
49694969
when(mockedPermissionsService.isUserOnProject(anything())).thenResolve(true);
4970+
when(mockedPermissionsService.canAccessText(anything(), anything())).thenReturn(true);
49704971
when(mockedFeatureFlagService.newDraftHistory).thenReturn(createTestFeatureFlag(false));
49714972
when(mockedLynxWorkspaceService.rawInsightSource$).thenReturn(of([]));
49724973

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts‎

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@ import { isParatextRole, SFProjectRole } from 'realtime-server/lib/esm/scripture
5858
import { TextAnchor } from 'realtime-server/lib/esm/scriptureforge/models/text-anchor';
5959
import { TextType } from 'realtime-server/lib/esm/scriptureforge/models/text-data';
6060
import { Chapter, TextInfo } from 'realtime-server/lib/esm/scriptureforge/models/text-info';
61-
import { TextInfoPermission } from 'realtime-server/lib/esm/scriptureforge/models/text-info-permission';
6261
import { TranslateSource } from 'realtime-server/lib/esm/scriptureforge/models/translate-config';
6362
import { fromVerseRef } from 'realtime-server/lib/esm/scriptureforge/models/verse-ref-data';
6463
import { DeltaOperation } from 'rich-text';
@@ -323,7 +322,6 @@ export class EditorComponent extends DataLoadingComponent implements OnDestroy,
323322
private isParatextUserRole: boolean = false;
324323
private projectUserConfigChangesSub?: Subscription;
325324
private text?: TextInfo;
326-
private sourceText?: TextInfo;
327325
sourceProjectDoc?: SFProjectProfileDoc;
328326
private _unsupportedTags = new Set<string>();
329327
private sourceLoaded: boolean = false;
@@ -530,24 +528,12 @@ export class EditorComponent extends DataLoadingComponent implements OnDestroy,
530528

531529
get hasSourceViewRight(): boolean {
532530
const sourceProject = this.sourceProjectDoc?.data;
533-
if (sourceProject == null) {
531+
const sourceId = this.source?.id;
532+
if (sourceProject == null || sourceId == null) {
534533
return this.isParatextUserRole;
535534
}
536535

537-
if (
538-
SF_PROJECT_RIGHTS.hasRight(sourceProject, this.userService.currentUserId, SFProjectDomain.Texts, Operation.View)
539-
) {
540-
// Check for chapter rights
541-
const chapter = this.sourceText?.chapters.find(c => c.number === this.chapter);
542-
// Even though permissions is guaranteed to be there in the model, its not in IndexedDB the first time the project
543-
// is accessed after migration
544-
if (chapter != null && chapter.permissions != null && !this.isParatextUserRole) {
545-
const chapterPermission: string = chapter.permissions[this.userService.currentUserId];
546-
return chapterPermission === TextInfoPermission.Write || chapterPermission === TextInfoPermission.Read;
547-
}
548-
}
549-
550-
return this.isParatextUserRole;
536+
return this.isParatextUserRole && this.permissionsService.canAccessText(sourceId, sourceProject);
551537
}
552538

553539
get canInsertNote(): boolean {
@@ -844,10 +830,6 @@ export class EditorComponent extends DataLoadingComponent implements OnDestroy,
844830
return;
845831
}
846832

847-
if (this.sourceProjectDoc?.data != null) {
848-
this.sourceText = this.sourceProjectDoc.data.texts.find(t => t.bookNum === bookNum);
849-
}
850-
851833
this.chapters = this.text.chapters.map(c => c.number);
852834

853835
this.updateVerseNumber();

0 commit comments

Comments
 (0)