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 password enforce on public share links #12273

Merged
merged 3 commits into from
Nov 6, 2018

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Nov 5, 2018

Fix #12272

New share id was different, therefore the popover check was not triggered.

@skjnldsv skjnldsv added bug 3. to review Waiting for reviews labels Nov 5, 2018
@skjnldsv skjnldsv added this to the Nextcloud 15 milestone Nov 5, 2018
@skjnldsv skjnldsv self-assigned this Nov 5, 2018
@rullzer
Copy link
Member

rullzer commented Nov 5, 2018

You need to run the hanldebars script 😉

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the public-share-enforce-pass-fix branch from 1cf8114 to 11b6c22 Compare November 5, 2018 09:50
@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 5, 2018

You need to run the hanldebars script

It's even worse, I forgot to add most of what I changed 🙈 😁

@schiessle
Copy link
Member

I tested it and it works almost 😉

If I have enabled the password policy app and use a password within the most used passwords, e.g. "helloworld" I get this:

image

and for a short moment a error banner at the top "unable to create share link". The response contains the correct error message from the password policy app, but it seems like we don't read it and display it at the moment.

@schiessle
Copy link
Member

Another small issue: It would be nice to put the focus directly on the input field so that the user can start directly typing.

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 6, 2018
@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 6, 2018

@schiessle Thanks!!
I'll fix this today ;)

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 6, 2018

Done!

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 6, 2018
@MorrisJobke MorrisJobke mentioned this pull request Nov 6, 2018
29 tasks
Copy link
Member

@schiessle schiessle left a comment

Choose a reason for hiding this comment

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

@skjnldsv I tested it... works great! 👍

@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 6, 2018

Most of the acceptance tests failures are unrelated: sh: 1: kill: No such process
Only the files looks suspicious, let me check

EDIT: passed on my setup, restarted the tests.

@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 6, 2018

@danxuliu can you take a quick look? I'm confused.
And I write down the shared link fails on one test but is fine on the others. Is this unrelated? I don't want to merge a failed test :)

Scenario: create folder in a public editable shared folder        # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-files.feature:170
    Given I act as John                                             # ActorContext::iActAs()
    And I am logged in                                              # LoginPageContext::iAmLoggedIn()
    And I create a new folder named "Editable shared folder"        # FileListContext::iCreateANewFolderNamed()
    And I close the details view                                    # FilesAppContext::iCloseTheDetailsView()
      │ Close details view in Files app could not be clicked
      │ Exception message: Offset within element cannot be scrolled into view: (0, 0): http://acceptance-app-files/index.php/apps/files/?dir=/&fileid=40#
      │ Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
      │ System info: host: 'c510756f7fe7', ip: '172.17.0.11', os.name: 'Linux', os.arch: 'amd64', os.version: '4.15.0-38-generic', java.version: '1.8.0_91'
      │ Driver info: driver.version: unknown
      │ Trying again
      │ 
    And I see that the details view is closed                       # FilesAppContext::iSeeThatTheDetailsViewIsClosed()
    And I share the link for "Editable shared folder"               # FilesAppContext::iShareTheLinkFor()
    And I set the shared link as editable                           # FilesAppContext::iSetTheSharedLinkAsEditable()
    And I write down the shared link                                # FilesAppContext::iWriteDownTheSharedLink()
      │ Copy link button in the details view in Files app could not be clicked
      │ Exception message: Element is no longer attached to the DOM
      │ For documentation on this error, please visit: http://seleniumhq.org/exceptions/stale_element_reference.html
      │ Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
      │ System info: host: 'c510756f7fe7', ip: '172.17.0.11', os.name: 'Linux', os.arch: 'amd64', os.version: '4.15.0-38-generic', java.version: '1.8.0_91'
      │ Driver info: driver.version: unknown
      │ Trying again
      │ 
    When I act as Jane                                              # ActorContext::iActAs()
    And I visit the shared link I wrote down                        # FilesSharingAppContext::iVisitTheSharedLinkIWroteDown()
    And I see that the current page is the shared link I wrote down # FilesSharingAppContext::iSeeThatTheCurrentPageIsTheSharedLinkIWroteDown()
    And I create a new folder named "Subfolder"                     # FileListContext::iCreateANewFolderNamed()
    Then I see that the file list contains a file named "Subfolder" # FileListContext::iSeeThatTheFileListContainsAFileNamed()

  Scenario: owner sees folder created in the public page of an editable shared folder # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-files.feature:191
    Given I act as John                                                               # ActorContext::iActAs()
    And I am logged in                                                                # LoginPageContext::iAmLoggedIn()
    And I create a new folder named "Editable shared folder"                          # FileListContext::iCreateANewFolderNamed()
    And I close the details view                                                      # FilesAppContext::iCloseTheDetailsView()
      │ Close details view in Files app could not be clicked
      │ Exception message: Offset within element cannot be scrolled into view: (0, 0): http://acceptance-app-files/index.php/apps/files/?dir=/&fileid=40#
      │ Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
      │ System info: host: 'c510756f7fe7', ip: '172.17.0.11', os.name: 'Linux', os.arch: 'amd64', os.version: '4.15.0-38-generic', java.version: '1.8.0_91'
      │ Driver info: driver.version: unknown
      │ Trying again
      │ 
    And I see that the details view is closed                                         # FilesAppContext::iSeeThatTheDetailsViewIsClosed()
    And I share the link for "Editable shared folder"                                 # FilesAppContext::iShareTheLinkFor()
    And I set the shared link as editable                                             # FilesAppContext::iSetTheSharedLinkAsEditable()
    And I write down the shared link                                                  # FilesAppContext::iWriteDownTheSharedLink()
      Element is no longer attached to the DOM
      For documentation on this error, please visit: http://seleniumhq.org/exceptions/stale_element_reference.html
      Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
      System info: host: 'c510756f7fe7', ip: '172.17.0.11', os.name: 'Linux', os.arch: 'amd64', os.version: '4.15.0-38-generic', java.version: '1.8.0_91'
      Driver info: driver.version: unknown (WebDriver\Exception\StaleElementReference)
    And I act as Jane                                                                 # ActorContext::iActAs()
    And I visit the shared link I wrote down                                          # FilesSharingAppContext::iVisitTheSharedLinkIWroteDown()
    And I see that the current page is the shared link I wrote down                   # FilesSharingAppContext::iSeeThatTheCurrentPageIsTheSharedLinkIWroteDown()
    And I create a new folder named "Subfolder"                                       # FileListContext::iCreateANewFolderNamed()
    And I see that the file list contains a file named "Subfolder"                    # FileListContext::iSeeThatTheFileListContainsAFileNamed()
    When I act as John                                                                # ActorContext::iActAs()
    And I enter in the folder named "Editable shared folder"                          # FileListContext::iEnterInTheFolderNamed()
    Then I see that the file list contains a file named "Subfolder"                   # FileListContext::iSeeThatTheFileListContainsAFileNamed()

@danxuliu
Copy link
Member

danxuliu commented Nov 6, 2018

And I write down the shared link fails on one test but is fine on the others. Is this unrelated?

Yes, it is a hiccup. We may start to see this failure more often due to the redesigned UI for link shares; if that happens I will think how to fix it, but for now I hope that it is just an infrequent issue ;-)

The problem is that clicking on the link modifies the link element to change its title from Copy link to Copied and back to Copy link; I have not checked it, but it is very likely that the element is also detached and attached again. The acceptance test gets the data-clipboard-text attribute from the link element to know the URL (as it can not access the clipboard), but if the element is modified (and probably detached and attached again) between the element was got and its attribute was got the reference to the element used by Selenium to get the attribute is no longer valid (that is the stale element reference mentioned in the output).

The tests are shielded against that due to using a wrapped element, but there is no support in the wrapper to get an attribute, so the pure Selenium object is used instead and due to that the problem can arise.

I don't want to merge a failed test :)

Good, you should not ;-) But in this case, feel free to merge :-)

@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 6, 2018

The acceptance test gets the data-clipboard-text attribute from the link element to know the URL

We should then use the input instead? :)
But it's hidden, so maybe it's not working?

Anyway, merging 😁

@skjnldsv skjnldsv merged commit 0f1edd5 into master Nov 6, 2018
@skjnldsv skjnldsv deleted the public-share-enforce-pass-fix branch November 6, 2018 17:28
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.

5 participants