Skip to content

Commit 07e0760

Browse files
pmachapmanpmachapman
authored
Revert "SF-3637 Deduplicate permissions (#3566)" (#3591)
1 parent 79b210d commit 07e0760

File tree

17 files changed

+142
-264
lines changed

17 files changed

+142
-264
lines changed

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

Lines changed: 11 additions & 13 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.canAccessTextAsync(cloneDeep(textDoc) as TextDocId)).toBe(true);
108+
expect(await env.service.canAccessText(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, false);
113+
env.setupProjectData(undefined);
114114

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

117-
expect(await env.service.canAccessTextAsync(cloneDeep(textDoc) as TextDocId)).toBe(true);
117+
expect(await env.service.canAccessText(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.canAccessTextAsync(cloneDeep(textDoc) as TextDocId)).toBe(false);
126+
expect(await env.service.canAccessText(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.canAccessTextAsync(cloneDeep(textDoc) as TextDocId)).toBe(false);
135+
expect(await env.service.canAccessText(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.canAccessTextAsync(cloneDeep(textDoc) as TextDocId)).toBe(false);
144+
expect(await env.service.canAccessText(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, chapterPermissions: boolean = true): void {
267+
setupProjectData(textPermission?: TextInfoPermission | undefined): void {
268268
const projectId: string = 'project01';
269269
const permission: TextInfoPermission = textPermission ?? TextInfoPermission.Write;
270270

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

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

Lines changed: 16 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
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';
43
import { SF_PROJECT_RIGHTS, SFProjectDomain } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-rights';
54
import { isParatextRole, SFProjectRole } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-role';
6-
import { Chapter, TextInfo } from 'realtime-server/lib/esm/scriptureforge/models/text-info';
5+
import { Chapter } from 'realtime-server/lib/esm/scriptureforge/models/text-info';
76
import { TextInfoPermission } from 'realtime-server/lib/esm/scriptureforge/models/text-info-permission';
87
import { UserDoc } from 'xforge-common/models/user-doc';
98
import { UserService } from 'xforge-common/user.service';
@@ -59,25 +58,25 @@ export class PermissionsService {
5958
return isParatextRole(projectDoc.data?.userRoles[currentUserDoc.id] ?? SFProjectRole.None);
6059
}
6160

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 {
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+
6969
// Ensure the user has project level permission to view the text
7070
if (
71-
project != null &&
72-
SF_PROJECT_RIGHTS.hasRight(project, this.userService.currentUserId, SFProjectDomain.Texts, Operation.View)
71+
projectDoc?.data != null &&
72+
SF_PROJECT_RIGHTS.hasRight(projectDoc.data, this.userService.currentUserId, SFProjectDomain.Texts, Operation.View)
7373
) {
7474
// Check chapter permissions
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];
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];
8180
// If there is no chapter permission, they will have access to the chapter as they have access to the project.
8281
// We should only deny access if there is an explicit "None" permission.
8382
return chapterPermission !== TextInfoPermission.None;
@@ -87,23 +86,6 @@ export class PermissionsService {
8786
return false;
8887
}
8988

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-
10789
canSync(projectDoc: SFProjectProfileDoc, userId?: string): boolean {
10890
if (projectDoc.data == null) {
10991
return false;

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

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

413-
it('should return false if the user does not have the permission', () => {
413+
it('should return undefined 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).toBe(false);
419+
expect(actual).toBeUndefined();
420420
});
421421

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

436-
it('should return true if the user has the write permission for the chapter', () => {
435+
it('should return true if the user has the write permission', () => {
437436
const env = new TestEnvironment();
438437
const text: Partial<TextInfo> = {
439438
chapters: [
@@ -445,18 +444,6 @@ describe('TextDocService', () => {
445444
const actual: boolean | undefined = env.textDocService.hasChapterEditPermissionForText(text as TextInfo, 1);
446445
expect(actual).toBe(true);
447446
});
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-
});
460447
});
461448
});
462449

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -187,12 +187,10 @@ 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;
191190
// Even though permissions is guaranteed to be there in the model, its not in IndexedDB the first time the project
192191
// is accessed after migration
193-
const permission: string | undefined =
194-
chapter?.permissions?.[this.userService.currentUserId] ?? text?.permissions?.[this.userService.currentUserId];
195-
return permission === TextInfoPermission.Write;
192+
const permission: string | undefined = chapter?.permissions?.[this.userService.currentUserId];
193+
return permission == null ? undefined : permission === TextInfoPermission.Write;
196194
}
197195

198196
/**

‎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.canAccessTextAsync(anything())).thenResolve(false);
27+
when(mockedPermissionService.canAccessText(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(
76-
mockedPermissionService.canAccessTextAsync(deepEqual(new TextDocId('sourceId', 0, 0, 'target')))
77-
).thenResolve(false); //remove access for one source doc
75+
when(mockedPermissionService.canAccessText(deepEqual(new TextDocId('sourceId', 0, 0, 'target')))).thenResolve(
76+
false
77+
); //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.canAccessTextAsync(anything())).thenResolve(true);
109+
when(mockedPermissionService.canAccessText(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.canAccessTextAsync(textDocId)) {
35+
if (await this.permissionsService.canAccessText(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.canAccessTextAsync(sourceTextDocId)) {
41+
if (await this.permissionsService.canAccessText(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.canAccessTextAsync(anything())).thenCall((textDocId: TextDocId) => {
142+
when(mockPermissionService.canAccessText(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.canAccessTextAsync(anything())).thenResolve(true);
195+
when(mockPermissionService.canAccessText(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.canAccessTextAsync(sourceTextDocId)) {
307+
if (await this.permissionsService.canAccessText(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: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4978,7 +4978,6 @@ class TestEnvironment {
49784978
when(mockedDraftGenerationService.getLastCompletedBuild(anything())).thenReturn(of({} as BuildDto));
49794979
when(mockedDraftOptionsService.areFormattingOptionsAvailableButUnselected(anything())).thenReturn(false);
49804980
when(mockedPermissionsService.isUserOnProject(anything())).thenResolve(true);
4981-
when(mockedPermissionsService.canAccessText(anything(), anything())).thenReturn(true);
49824981
when(mockedFeatureFlagService.newDraftHistory).thenReturn(createTestFeatureFlag(false));
49834982
when(mockedLynxWorkspaceService.rawInsightSource$).thenReturn(of([]));
49844983

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

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ 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';
6162
import { TranslateSource } from 'realtime-server/lib/esm/scriptureforge/models/translate-config';
6263
import { fromVerseRef } from 'realtime-server/lib/esm/scriptureforge/models/verse-ref-data';
6364
import { DeltaOperation } from 'rich-text';
@@ -316,6 +317,7 @@ export class EditorComponent extends DataLoadingComponent implements OnDestroy,
316317
private isParatextUserRole: boolean = false;
317318
private projectUserConfigChangesSub?: Subscription;
318319
private text?: TextInfo;
320+
private sourceText?: TextInfo;
319321
sourceProjectDoc?: SFProjectProfileDoc;
320322
private _unsupportedTags = new Set<string>();
321323
private sourceLoaded: boolean = false;
@@ -522,12 +524,24 @@ export class EditorComponent extends DataLoadingComponent implements OnDestroy,
522524

523525
get hasSourceViewRight(): boolean {
524526
const sourceProject = this.sourceProjectDoc?.data;
525-
const sourceId = this.source?.id;
526-
if (sourceProject == null || sourceId == null) {
527+
if (sourceProject == null) {
527528
return this.isParatextUserRole;
528529
}
529530

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

533547
get canInsertNote(): boolean {
@@ -824,6 +838,10 @@ export class EditorComponent extends DataLoadingComponent implements OnDestroy,
824838
return;
825839
}
826840

841+
if (this.sourceProjectDoc?.data != null) {
842+
this.sourceText = this.sourceProjectDoc.data.texts.find(t => t.bookNum === bookNum);
843+
}
844+
827845
this.chapters = this.text.chapters.map(c => c.number);
828846

829847
this.updateVerseNumber();

0 commit comments

Comments
 (0)