Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ids of permission checkboxes for shares #9237

Merged
merged 2 commits into from
Apr 19, 2018

Conversation

danxuliu
Copy link
Member

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, 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, which was therefore seen as the element of type spaces that is a descendant of the element of type with that is in turn descendant of the element with id canEdit-view1-name) and thus the initial state of the checkbox was not properly set; see first test case below.

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; see second test case below.

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; due to that it is no longer needed to escape the special characters.

How to test (1)

  • Create a user with the user name user name with spaces
  • Open the Sharing tab in the sidebar of the Files app
  • Share a file with that user

Expected result:
The file is shared and the Can edit checkbox is checked; it is possible to check and uncheck it.

Actual result:
The file is shared but the Can edit checkbox is not checked, and it is not possible to check and uncheck it.

How to test (2)

  • Create a user with the id user'
  • Open the Sharing tab in the sidebar of the Files app
  • Share a file with that user

Expected result:
The file is shared and you can keep adding other shares.

Actual result:
The file is shared but the input field to search for other sharees is kept disabled. Reloading the page and opening the Sharing tab again does not even show the input field.

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, 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 initial 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 <[email protected]>
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 <[email protected]>
@codecov
Copy link

codecov bot commented Apr 18, 2018

Codecov Report

Merging #9237 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #9237      +/-   ##
============================================
- Coverage     51.91%   51.91%   -0.01%     
  Complexity    25361    25361              
============================================
  Files          1606     1606              
  Lines         95311    95309       -2     
  Branches       1394     1394              
============================================
- Hits          49478    49475       -3     
- Misses        45833    45834       +1
Impacted Files Coverage Δ Complexity Δ
core/js/sharedialogshareelistview.js 48.42% <100%> (-0.36%) 0 <0> (ø)
apps/files_trashbin/lib/Trashbin.php 72.46% <0%> (-0.25%) 136% <0%> (ø)

@blizzz
Copy link
Member

blizzz commented Apr 19, 2018

if the share id is unique to different recipients of the same share, go for it

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested an works 👍

@MorrisJobke MorrisJobke merged commit 4d71e12 into master Apr 19, 2018
@MorrisJobke MorrisJobke deleted the fix-ids-of-permission-checkboxes-for-shares branch May 9, 2018 06:48
@MorrisJobke
Copy link
Member

🏓 regarding the backport 😉

@danxuliu
Copy link
Member Author

Backport is in #9453 ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants