From 0734325dda691f39504a5da0a6f1626192ab9571 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= <danxuliu@gmail.com>
Date: Wed, 18 Apr 2018 14:07:04 +0200
Subject: [PATCH 1/2] Fix ids of permission checkboxes for shares
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The ids of permission checkboxes for shares were generated using the
"shareWith" field of the share. The "shareWith" field can contain spaces
(as spaces are allowed, for example, in user or circle names), so this
could cause the id attribute of the HTML element to contain spaces too,
which is forbidden by the HTML specification.

It is not just a "formal" issue, though; when the list was rendered
after a permission change, if the id contained a space the selector to
get the checkbox element was wrong (as it ended being something like
"#canEdit-view1-name with spaces") and thus the updated state of the
checkbox was not properly set.

Besides that, "shareWith" can contain too single quotes, which would
even cause the jQuery selector to abort the search and leave the UI in
an invalid state.

Instead of adding more cases to the regular expression to escape special
characters and apply it too when the ids are created now the ids of
permission checkboxes for shares are based on the "shareId" field
instead of on "shareWith", as "shareId" is expected to always contain
compatible characters.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
---
 core/js/sharedialogshareelistview.js          | 22 ++++----
 .../tests/specs/sharedialogshareelistview.js  | 50 +++++++++++++++++++
 2 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/core/js/sharedialogshareelistview.js b/core/js/sharedialogshareelistview.js
index 8ec53810b5129..a88c38fb4eced 100644
--- a/core/js/sharedialogshareelistview.js
+++ b/core/js/sharedialogshareelistview.js
@@ -30,8 +30,8 @@
 					'<span class="sharingOptionsGroup">' +
 						'{{#if editPermissionPossible}}' +
 						'<span class="shareOption">' +
-							'<input id="canEdit-{{cid}}-{{shareWith}}" type="checkbox" name="edit" class="permissions checkbox" {{#if hasEditPermission}}checked="checked"{{/if}} />' +
-							'<label for="canEdit-{{cid}}-{{shareWith}}">{{canEditLabel}}</label>' +
+							'<input id="canEdit-{{cid}}-{{shareId}}" type="checkbox" name="edit" class="permissions checkbox" {{#if hasEditPermission}}checked="checked"{{/if}} />' +
+							'<label for="canEdit-{{cid}}-{{shareId}}">{{canEditLabel}}</label>' +
 						'</span>' +
 						'{{/if}}' +
 						'<a href="#"><span class="icon icon-more"></span></a>' +
@@ -58,8 +58,8 @@
 				'{{#if isResharingAllowed}} {{#if sharePermissionPossible}} {{#unless isMailShare}}' +
 				'<li>' +
 					'<span class="shareOption menuitem">' +
-						'<input id="canShare-{{cid}}-{{shareWith}}" type="checkbox" name="share" class="permissions checkbox" {{#if hasSharePermission}}checked="checked"{{/if}} data-permissions="{{sharePermission}}" />' +
-						'<label for="canShare-{{cid}}-{{shareWith}}">{{canShareLabel}}</label>' +
+						'<input id="canShare-{{cid}}-{{shareId}}" type="checkbox" name="share" class="permissions checkbox" {{#if hasSharePermission}}checked="checked"{{/if}} data-permissions="{{sharePermission}}" />' +
+						'<label for="canShare-{{cid}}-{{shareId}}">{{canShareLabel}}</label>' +
 					'</span>' +
 				'</li>' +
 				'{{/unless}} {{/if}} {{/if}}' +
@@ -67,24 +67,24 @@
 					'{{#if createPermissionPossible}}{{#unless isMailShare}}' +
 					'<li>' +
 						'<span class="shareOption menuitem">' +
-							'<input id="canCreate-{{cid}}-{{shareWith}}" type="checkbox" name="create" class="permissions checkbox" {{#if hasCreatePermission}}checked="checked"{{/if}} data-permissions="{{createPermission}}"/>' +
-							'<label for="canCreate-{{cid}}-{{shareWith}}">{{createPermissionLabel}}</label>' +
+							'<input id="canCreate-{{cid}}-{{shareId}}" type="checkbox" name="create" class="permissions checkbox" {{#if hasCreatePermission}}checked="checked"{{/if}} data-permissions="{{createPermission}}"/>' +
+							'<label for="canCreate-{{cid}}-{{shareId}}">{{createPermissionLabel}}</label>' +
 						'</span>' +
 					'</li>' +
 					'{{/unless}}{{/if}}' +
 					'{{#if updatePermissionPossible}}{{#unless isMailShare}}' +
 					'<li>' +
 						'<span class="shareOption menuitem">' +
-							'<input id="canUpdate-{{cid}}-{{shareWith}}" type="checkbox" name="update" class="permissions checkbox" {{#if hasUpdatePermission}}checked="checked"{{/if}} data-permissions="{{updatePermission}}"/>' +
-							'<label for="canUpdate-{{cid}}-{{shareWith}}">{{updatePermissionLabel}}</label>' +
+							'<input id="canUpdate-{{cid}}-{{shareId}}" type="checkbox" name="update" class="permissions checkbox" {{#if hasUpdatePermission}}checked="checked"{{/if}} data-permissions="{{updatePermission}}"/>' +
+							'<label for="canUpdate-{{cid}}-{{shareId}}">{{updatePermissionLabel}}</label>' +
 						'</span>' +
 					'</li>' +
 					'{{/unless}}{{/if}}' +
 					'{{#if deletePermissionPossible}}{{#unless isMailShare}}' +
 					'<li>' +
 						'<span class="shareOption menuitem">' +
-							'<input id="canDelete-{{cid}}-{{shareWith}}" type="checkbox" name="delete" class="permissions checkbox" {{#if hasDeletePermission}}checked="checked"{{/if}} data-permissions="{{deletePermission}}"/>' +
-							'<label for="canDelete-{{cid}}-{{shareWith}}">{{deletePermissionLabel}}</label>' +
+							'<input id="canDelete-{{cid}}-{{shareId}}" type="checkbox" name="delete" class="permissions checkbox" {{#if hasDeletePermission}}checked="checked"{{/if}} data-permissions="{{deletePermission}}"/>' +
+							'<label for="canDelete-{{cid}}-{{shareId}}">{{deletePermissionLabel}}</label>' +
 						'</span>' +
 					'</li>' +
 					'{{/unless}}{{/if}}' +
@@ -380,7 +380,7 @@
 				var $li = this.$('li[data-share-id=' + permissionChangeShareId + ']');
 				$li.find('.sharingOptionsGroup .popovermenu').replaceWith(this.popoverMenuTemplate(sharee));
 
-				var checkBoxId = 'canEdit-' + this.cid + '-' + sharee.shareWith;
+				var checkBoxId = 'canEdit-' + this.cid + '-' + sharee.shareId;
 				checkBoxId = '#' + checkBoxId.replace( /(:|\.|\[|\]|,|=|@)/g, "\\$1");
 				var $edit = $li.parent().find(checkBoxId);
 				if($edit.length === 1) {
diff --git a/core/js/tests/specs/sharedialogshareelistview.js b/core/js/tests/specs/sharedialogshareelistview.js
index 8ee2c48fe39c9..c1c10b6010771 100644
--- a/core/js/tests/specs/sharedialogshareelistview.js
+++ b/core/js/tests/specs/sharedialogshareelistview.js
@@ -122,6 +122,56 @@ describe('OC.Share.ShareDialogShareeListView', function () {
 			expect(listView.$el.find("input[name='edit']").is(':checked')).toEqual(true);
 			expect(updateShareStub.calledOnce).toEqual(true);
 		});
+
+		it('Checks edit box when clicking on it', function () {
+			updateShareStub.callsFake(function (shareId, attributes) {
+				// Update the permissions to get the new value when rendering
+				// again.
+				var shareIndex = this.findShareWithIndex(shareId);
+				this.get('shares')[shareIndex].permissions = attributes.permissions;
+			});
+
+			shareModel.set('shares', [{
+				id: 100,
+				item_source: 123,
+				permissions: 0,
+				share_type: OC.Share.SHARE_TYPE_USER,
+				share_with: 'user1',
+				share_with_displayname: 'User One'
+			}]);
+			shareModel.set('itemType', 'folder');
+			listView.render();
+			expect(listView.$el.find("input[name='edit']").is(':checked')).toEqual(false);
+			listView.$el.find("input[name='edit']").click();
+			listView.render();
+			expect(listView.$el.find("input[name='edit']").is(':checked')).toEqual(true);
+			expect(updateShareStub.calledOnce).toEqual(true);
+		});
+
+		it('Checks edit box when clicking on it for sharee with special characters', function () {
+			updateShareStub.callsFake(function (shareId, attributes) {
+				// Update the permissions to get the new value when rendering
+				// again.
+				var shareIndex = this.findShareWithIndex(shareId);
+				this.get('shares')[shareIndex].permissions = attributes.permissions;
+			});
+
+			shareModel.set('shares', [{
+				id: 100,
+				item_source: 123,
+				permissions: 0,
+				share_type: OC.Share.SHARE_TYPE_USER,
+				share_with: 'user _.@-\'',
+				share_with_displayname: 'User One'
+			}]);
+			shareModel.set('itemType', 'folder');
+			listView.render();
+			expect(listView.$el.find("input[name='edit']").is(':checked')).toEqual(false);
+			listView.$el.find("input[name='edit']").click();
+			listView.render();
+			expect(listView.$el.find("input[name='edit']").is(':checked')).toEqual(true);
+			expect(updateShareStub.calledOnce).toEqual(true);
+		});
 	});
 
 });

From 2b5d933b63ad7b1e5401c146724d7694ed28f858 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= <danxuliu@gmail.com>
Date: Wed, 18 Apr 2018 17:03:45 +0200
Subject: [PATCH 2/2] Remove no longer needed escaping of special characters
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The escaping of special characters was needed when the ids of the
permission checkboxes for shares were based on the "shareWith" field.
Since they are based on the "shareId" field the escaping is no longer
needed, as the "sharedId" is expected to always contain compatible
characters.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
---
 core/js/sharedialogshareelistview.js | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/core/js/sharedialogshareelistview.js b/core/js/sharedialogshareelistview.js
index a88c38fb4eced..6e5248e83eb30 100644
--- a/core/js/sharedialogshareelistview.js
+++ b/core/js/sharedialogshareelistview.js
@@ -380,9 +380,7 @@
 				var $li = this.$('li[data-share-id=' + permissionChangeShareId + ']');
 				$li.find('.sharingOptionsGroup .popovermenu').replaceWith(this.popoverMenuTemplate(sharee));
 
-				var checkBoxId = 'canEdit-' + this.cid + '-' + sharee.shareId;
-				checkBoxId = '#' + checkBoxId.replace( /(:|\.|\[|\]|,|=|@)/g, "\\$1");
-				var $edit = $li.parent().find(checkBoxId);
+				var $edit = $li.parent().find('#canEdit-' + this.cid + '-' + sharee.shareId);
 				if($edit.length === 1) {
 					$edit.prop('checked', sharee.hasEditPermission);
 				}