Skip to content

Commit

Permalink
New: Separate create and view permissions for annotations (#358)
Browse files Browse the repository at this point in the history
Enables annotations to be shown in a view-only setting with the
correct scopes

Annotations permissions will be as follows:
- can_annotate - whether a user can add any new annotations
- can_view_annotations_all - whether a user can view all annotations
- can_view_annotations_self - whether a user can view their own
  annotations (This is not yet supported as Preview is not aware of
  who the user previewing the file is in order to distinguish which
  annotations belong to the current user vs. other users)
  • Loading branch information
pramodsum authored Sep 1, 2017
1 parent 8f7c2df commit 5d39f1f
Show file tree
Hide file tree
Showing 15 changed files with 111 additions and 66 deletions.
1 change: 1 addition & 0 deletions src/lib/annotations/AnnotationThread.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class AnnotationThread extends EventEmitter {
this.type = data.type;
this.locale = data.locale;
this.isMobile = data.isMobile;
this.permissions = data.permissions;

this.setup();
}
Expand Down
23 changes: 18 additions & 5 deletions src/lib/annotations/Annotator.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ class Annotator extends EventEmitter {
constructor(data) {
super();

this.canAnnotate = data.canAnnotate;
this.container = data.container;
this.options = data.options;
this.fileVersionId = data.fileVersionId;
Expand All @@ -65,6 +64,14 @@ class Annotator extends EventEmitter {

const { CONTROLLERS } = this.options.annotator || {};
this.modeControllers = CONTROLLERS || {};

// Get annotation permissions
const permissions = data.options.permissions || {};
this.permissions = {
canAnnotate: permissions.can_annotate || false,
canViewAllAnnotations: permissions.can_view_annotations_all || false,
canViewOwnAnnotations: permissions.can_view_annotations_self || false
};
}

/**
Expand Down Expand Up @@ -112,8 +119,7 @@ class Annotator extends EventEmitter {
this.annotationService = new AnnotationService({
apiHost,
fileId,
token,
canAnnotate: this.canAnnotate
token
});

// Set up mobile annotations dialog
Expand Down Expand Up @@ -162,7 +168,7 @@ class Annotator extends EventEmitter {
*/
showModeAnnotateButton(currentMode) {
const mode = this.modeButtons[currentMode];
if (!mode || !this.isModeAnnotatable(currentMode)) {
if (!mode || !this.permissions.canAnnotate || !this.isModeAnnotatable(currentMode)) {
return;
}

Expand Down Expand Up @@ -314,7 +320,7 @@ class Annotator extends EventEmitter {

// Only show/hide point annotation button if user has the
// appropriate permissions
if (!this.canAnnotate) {
if (!this.permissions.canAnnotate) {
return;
}

Expand Down Expand Up @@ -534,11 +540,18 @@ class Annotator extends EventEmitter {
fetchAnnotations() {
this.threads = {};

// Do not load any pre-existing annotations if the user does not have
// the correct permissions
if (!this.permissions.canViewAllAnnotations || !this.permissions.canViewOwnAnnotations) {
return this.threads;
}

return this.annotationService.getThreadMap(this.fileVersionId).then((threadMap) => {
// Generate map of page to threads
Object.keys(threadMap).forEach((threadID) => {
const annotations = threadMap[threadID];
const firstAnnotation = annotations[0];

if (!firstAnnotation || !this.isModeAnnotatable(firstAnnotation.type)) {
return;
}
Expand Down
38 changes: 34 additions & 4 deletions src/lib/annotations/__tests__/Annotator-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ describe('lib/annotations/Annotator', () => {
stubs.setupMobileDialog = sandbox.stub(annotator, 'setupMobileDialog');
stubs.showButton = sandbox.stub(annotator, 'showModeAnnotateButton');

annotator.canAnnotate = true;
annotator.permissions.canAnnotate = true;
annotator.modeButtons = {
point: { selector: 'point_btn' },
draw: { selector: 'draw_btn' }
Expand Down Expand Up @@ -242,7 +242,7 @@ describe('lib/annotations/Annotator', () => {

describe('rotateAnnotations()', () => {
beforeEach(() => {
annotator.canAnnotate = true;
annotator.permissions.canAnnotate = true;
stubs.hide = sandbox.stub(annotatorUtil, 'hideElement');
stubs.show = sandbox.stub(annotatorUtil, 'showElement');
stubs.render = sandbox.stub(annotator, 'renderAnnotations');
Expand All @@ -258,7 +258,7 @@ describe('lib/annotations/Annotator', () => {
});

it('should only render annotations if user cannot annotate', () => {
annotator.canAnnotate = false;
annotator.permissions.canAnnotate = false;
annotator.rotateAnnotations();
expect(stubs.hide).to.not.be.called;
expect(stubs.show).to.not.be.called;
Expand Down Expand Up @@ -419,12 +419,32 @@ describe('lib/annotations/Annotator', () => {
someID2: [{}]
};
stubs.threadPromise = Promise.resolve(threadMap);
stubs.serviceMock.expects('getThreadMap').returns(stubs.threadPromise);
sandbox.stub(annotator, 'emit');
sandbox.stub(annotator, 'isModeAnnotatable').returns(true);

annotator.permissions = {
canViewAllAnnotations: true,
canViewOwnAnnotations: true
};
});

it('should not fetch existing annotations if the user does not have correct permissions', () => {
stubs.serviceMock.expects('getThreadMap').never();
annotator.permissions = {
canViewAllAnnotations: false,
canViewOwnAnnotations: true
};
annotator.fetchAnnotations();

annotator.permissions = {
canViewAllAnnotations: true,
canViewOwnAnnotations: false
};
annotator.fetchAnnotations();
});

it('should reset and create a new thread map by fetching annotation data from the server', () => {
stubs.serviceMock.expects('getThreadMap').returns(stubs.threadPromise);
stubs.createThread = sandbox.stub(annotator, 'createAnnotationThread');
stubs.createThread.onFirstCall();
stubs.createThread.onSecondCall().returns(stubs.thread);
Expand All @@ -440,6 +460,7 @@ describe('lib/annotations/Annotator', () => {
});

it('should emit a message to indicate that all annotations have been fetched', () => {
stubs.serviceMock.expects('getThreadMap').returns(stubs.threadPromise);
annotator.fetchAnnotations();
return stubs.threadPromise.then(() => {
expect(annotator.emit).to.be.calledWith('annotationsfetched');
Expand Down Expand Up @@ -911,13 +932,22 @@ describe('lib/annotations/Annotator', () => {
selector: '.bp-btn-annotate'
}
};
annotator.permissions.canAnnotate = true;
});

afterEach(() => {
annotator.modeButtons = {};
annotator.container = document;
});

it('should do nothing if user cannot annotate', () => {
annotator.permissions.canAnnotate = false;
sandbox.stub(annotator, 'isModeAnnotatable').returns(true);
sandbox.stub(annotator, 'getAnnotationModeClickHandler');
annotator.showModeAnnotateButton(TYPES.point);
expect(annotator.getAnnotationModeClickHandler).to.not.be.called;
});

it('should do nothing if the mode does not require a button', () => {
sandbox.stub(annotator, 'getAnnotationModeClickHandler');
sandbox.stub(annotator, 'isModeAnnotatable').returns(true);
Expand Down
11 changes: 2 additions & 9 deletions src/lib/annotations/doc/DocAnnotator.js
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,8 @@ class DocAnnotator extends Annotator {
isMobile: this.isMobile,
locale: this.locale,
location,
type
type,
permissions: this.permissions
};

// Set existing thread ID if created with annotations
Expand Down Expand Up @@ -455,10 +456,6 @@ class DocAnnotator extends Annotator {

this.annotatedElement.addEventListener('mouseup', this.highlightMouseupHandler);

if (!this.canAnnotate) {
return;
}

if (this.hasTouch && this.isMobile) {
document.addEventListener('selectionchange', this.onSelectionChange);
this.annotatedElement.addEventListener('touchstart', this.drawingSelectionHandler);
Expand Down Expand Up @@ -488,10 +485,6 @@ class DocAnnotator extends Annotator {
this.highlightThrottleHandle = null;
}

if (!this.canAnnotate) {
return;
}

if (this.hasTouch && this.isMobile) {
document.removeEventListener('selectionchange', this.onSelectionChange);
this.annotatedElement.removeEventListener('touchstart', this.drawingSelectionHandler);
Expand Down
11 changes: 7 additions & 4 deletions src/lib/annotations/doc/DocHighlightDialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -293,9 +293,12 @@ class DocHighlightDialog extends AnnotationDialog {
]);
annotatorUtil.showElement(highlightLabelEl);

// Hide delete button on plain highlights if user doesn't have
// permissions
if (annotations[0].permissions && !annotations[0].permissions.can_delete) {
if (!this.canAnnotate) {
// Hide all action buttons if user cannot annotate
const highlightButtons = this.highlightDialogEl.querySelector(constants.SELECTOR_HIGHLIGHT_BTNS);
annotatorUtil.hideElement(highlightButtons);
} else if (annotations[0].permissions && !annotations[0].permissions.can_delete) {
// Hide delete button on plain highlights if user doesn't have permissions
const addHighlightBtn = this.highlightDialogEl.querySelector(constants.SELECTOR_ADD_HIGHLIGHT_BTN);
annotatorUtil.hideElement(addHighlightBtn);
}
Expand All @@ -306,7 +309,7 @@ class DocHighlightDialog extends AnnotationDialog {
this.addAnnotationElement(annotation);
});

if (!this.isMobile) {
if (!this.isMobile && this.canAnnotate) {
this.bindDOMListeners();
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/lib/annotations/doc/DocHighlightThread.js
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ class DocHighlightThread extends AnnotationThread {
annotations: this.annotations,
locale: this.locale,
location: this.location,
canAnnotate: this.annotationService.canAnnotate
canAnnotate: this.permissions.canAnnotate
});

// Ensures that previously created annotations have the right type
Expand Down
4 changes: 2 additions & 2 deletions src/lib/annotations/doc/DocPointThread.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class DocPointThread extends AnnotationThread {
*/
showDialog() {
// Don't show dialog if user can annotate and there is a current selection
if (this.annotationService.canAnnotate && docAnnotatorUtil.isSelectionPresent()) {
if (this.permissions.canAnnotate && docAnnotatorUtil.isSelectionPresent()) {
return;
}

Expand Down Expand Up @@ -78,7 +78,7 @@ class DocPointThread extends AnnotationThread {
annotations: this.annotations,
locale: this.locale,
location: this.location,
canAnnotate: this.annotationService.canAnnotate
canAnnotate: this.permissions.canAnnotate
});
}
}
Expand Down
29 changes: 2 additions & 27 deletions src/lib/annotations/doc/__tests__/DocAnnotator-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -530,19 +530,6 @@ describe('lib/annotations/doc/DocAnnotator', () => {
stubs.elMock = sandbox.mock(annotator.annotatedElement);
});

it('shouldn\'t bind DOM listeners if user cannot annotate except mouseup', () => {
annotator.canAnnotate = false;

stubs.elMock.expects('addEventListener').withArgs('mouseup', sinon.match.func);
stubs.elMock.expects('addEventListener').withArgs('dblclick', sinon.match.func).never();
stubs.elMock.expects('addEventListener').withArgs('mousedown', sinon.match.func).never();
stubs.elMock.expects('addEventListener').withArgs('contextmenu', sinon.match.func).never();
stubs.elMock.expects('addEventListener').withArgs('mousemove', sinon.match.func).never();
stubs.elMock.expects('addEventListener').withArgs('touchstart', sinon.match.func).never();
stubs.elMock.expects('addEventListener').withArgs('click', sinon.match.func).never();
annotator.bindDOMListeners();
});

it('should bind DOM listeners if user can annotate', () => {
annotator.canAnnotate = true;

Expand All @@ -556,7 +543,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
});

it('should bind selectionchange event, on the document, if on mobile and can annotate', () => {
annotator.annotationService.canAnnotate = true;
annotator.permissions.canAnnotate = true;
annotator.isMobile = true;
annotator.hasTouch = true;
const docListen = sandbox.spy(document, 'addEventListener');
Expand All @@ -578,18 +565,6 @@ describe('lib/annotations/doc/DocAnnotator', () => {
annotator.highlightMousemoveHandler = () => {};
});

it('should not unbind DOM listeners if user cannot annotate except mouseup', () => {
annotator.canAnnotate = false;

stubs.elMock.expects('removeEventListener').withArgs('mouseup', sinon.match.func);
stubs.elMock.expects('removeEventListener').withArgs('mousedown', sinon.match.func).never();
stubs.elMock.expects('removeEventListener').withArgs('contextmenu', sinon.match.func).never();
stubs.elMock.expects('removeEventListener').withArgs('mousemove', sinon.match.func).never();
stubs.elMock.expects('removeEventListener').withArgs('dblclick', sinon.match.func).never();
stubs.elMock.expects('removeEventListener').withArgs('click', sinon.match.func).never();
annotator.unbindDOMListeners();
});

it('should unbind DOM listeners if user can annotate', () => {
annotator.canAnnotate = true;

Expand All @@ -616,7 +591,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
});

it('should unbind selectionchange event, on the document, if on mobile, has touch and can annotate', () => {
annotator.annotationService.canAnnotate = true;
annotator.permissions.canAnnotate = true;
annotator.isMobile = true;
annotator.hasTouch = true;
const docStopListen = sandbox.spy(document, 'removeEventListener');
Expand Down
15 changes: 14 additions & 1 deletion src/lib/annotations/doc/__tests__/DocHighlightDialog-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,20 @@ describe('lib/annotations/doc/DocHighlightDialog', () => {
expect(stubs.show).to.be.called;
});

it('should hide delete button on plain highlights if user does not have permissions', () => {
it('should hide all buttons on plain highlights if user does not have canAnnotate permissions', () => {
sandbox.stub(annotatorUtil, 'isPlainHighlight').returns(true);
dialog.canAnnotate = false;

dialog.setup([stubs.annotation]);
const highlightLabelEl = dialog.highlightDialogEl.querySelector(`.${CLASS_HIGHLIGHT_LABEL}`);
const highlightBtns = dialog.highlightDialogEl.querySelector(constants.SELECTOR_HIGHLIGHT_BTNS);
const addHighlightBtn = dialog.highlightDialogEl.querySelector(constants.SELECTOR_ADD_HIGHLIGHT_BTN);
expect(stubs.show).to.be.calledWith(highlightLabelEl);
expect(stubs.hide).to.be.calledWith(highlightBtns);
expect(stubs.hide).to.not.be.calledWith(addHighlightBtn);
});

it('should hide delete button on plain highlights if user does not have delete permissions', () => {
sandbox.stub(annotatorUtil, 'isPlainHighlight').returns(true);
stubs.annotation.permissions.can_delete = false;

Expand Down
18 changes: 15 additions & 3 deletions src/lib/annotations/doc/__tests__/DocHighlightThread-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ describe('lib/annotations/doc/DocHighlightThread', () => {
fileVersionId: 1,
location: {},
threadID: 2,
type: 'highlight'
type: 'highlight',
permissions: {
canAnnotate: true,
canViewAllAnnotations: true
}
});
highlightThread.dialog.setup([]);
});
Expand Down Expand Up @@ -151,7 +155,11 @@ describe('lib/annotations/doc/DocHighlightThread', () => {
fileVersionId: 1,
location: {},
threadID: 2,
type: 'highlight'
type: 'highlight',
permissions: {
canAnnotate: true,
canViewAllAnnotations: true
}
});
plainHighlightThread.dialog.setup([]);

Expand All @@ -178,7 +186,11 @@ describe('lib/annotations/doc/DocHighlightThread', () => {
fileVersionId: 1,
location: {},
threadID: 2,
type: 'highlight'
type: 'highlight',
permissions: {
canAnnotate: true,
canViewAllAnnotations: true
}
});

Object.defineProperty(Object.getPrototypeOf(DocHighlightThread.prototype), 'deleteAnnotation', {
Expand Down
Loading

0 comments on commit 5d39f1f

Please sign in to comment.