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

Additional perm check in Webdav #25388

Merged
merged 2 commits into from
Jul 12, 2016
Merged

Additional perm check in Webdav #25388

merged 2 commits into from
Jul 12, 2016

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Jul 6, 2016

@PVince81 PVince81 added this to the 9.2 milestone Jul 6, 2016
@mention-bot
Copy link

@PVince81, thanks for your PR! By analyzing the annotation information on this pull request, we identified @SergioBertolinSG, @rullzer, @icewind1991 and @DeepDiver1975 to be potential reviewers

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 7, 2016

Unit tests added.

Please review @SergioBertolinSG @DeepDiver1975 @guruz @georgehrke @VicDeo

@SergioBertolinSG
Copy link
Contributor

SergioBertolinSG commented Jul 7, 2016

👎 integration tests are not working fine:

Some are skipped:

 FeatureContext has missing steps. Define them with these snippets:


    /**
     * @Given :arg1 copies file :arg2 to :arg3
     */
    public function copiesFileTo($arg1, $arg2, $arg3)
    {
        throw new PendingException();
    }

    /**
     * @Given user :arg1 copies file :arg2 to :arg3
     */
    public function userCopiesFileTo($arg1, $arg2, $arg3)
    {
        throw new PendingException();
    }

| shareType | 0 |
| permissions | 1 |
| shareWith | user0 |
And "user1" copies file "/textfile0.txt" to "/testshare/overwritethis.txt"
Copy link
Contributor

@SergioBertolinSG SergioBertolinSG Jul 7, 2016

Choose a reason for hiding this comment

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

Missing 'User' before "user1"

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 7, 2016

Strange think, I'm pretty sure I ran these tests individually.
Will have a look, thanks for the hints.

@PVince81 PVince81 force-pushed the webdav-copypermissions branch from 2b33227 to f8d7757 Compare July 7, 2016 13:30
@PVince81
Copy link
Contributor Author

@icewind1991
Copy link
Contributor

Looks good 👍

@DeepDiver1975
Copy link
Member

jsunit test errors:

11:20:14 PhantomJS 2.1.1 (Linux 0.0.0) OCA.Files.FileList tests loading file list allows paths with dotdot at the beginning or end FAILED
11:20:14    Expected '/..abc' to equal '..abc'.
11:20:14    /ssd/jenkins/workspace/core-ci-linux-jsunit/database/sqlite/label/SLAVE/apps/files/tests/js/filelistSpec.js:1359:51
11:20:14    forEach@/ssd/jenkins/workspace/core-ci-linux-jsunit/database/sqlite/label/SLAVE/core/vendor/underscore/underscore.js:153:17
11:20:14    /ssd/jenkins/workspace/core-ci-linux-jsunit/database/sqlite/label/SLAVE/apps/files/tests/js/filelistSpec.js:1353:10
11:20:14    Expected '/def..' to equal 'def..'.
11:20:14    /ssd/jenkins/workspace/core-ci-linux-jsunit/database/sqlite/label/SLAVE/apps/files/tests/js/filelistSpec.js:1359:51
11:20:14    forEach@/ssd/jenkins/workspace/core-ci-linux-jsunit/database/sqlite/label/SLAVE/core/vendor/underscore/underscore.js:153:17
11:20:14    /ssd/jenkins/workspace/core-ci-linux-jsunit/database/sqlite/label/SLAVE/apps/files/tests/js/filelistSpec.js:1353:10
11:20:14    Expected '/...' to equal '...'.
11:20:14    /ssd/jenkins/workspace/core-ci-linux-jsunit/database/sqlite/label/SLAVE/apps/files/tests/js/filelistSpec.js:1359:51
11:20:14    forEach@/ssd/jenkins/workspace/core-ci-linux-jsunit/database/sqlite/label/SLAVE/core/vendor/underscore/underscore.js:153:17
11:20:14    /ssd/jenkins/workspace/core-ci-linux-jsunit/database/sqlite/label/SLAVE/apps/files/tests/js/filelistSpec.js:1353:10

@PVince81
Copy link
Contributor Author

@DeepDiver1975 ah yes, that one was failing on master. Will rebase.

@PVince81 PVince81 force-pushed the webdav-copypermissions branch from f8d7757 to bfaaff0 Compare July 11, 2016 09:00
@PVince81
Copy link
Contributor Author

Rebased. The JS tests passed locally.

@SergioBertolinSG
Copy link
Contributor

Integration tests are fine. But why checking the http answer instead of downloading the destination files? like here https://github.com/owncloud/core/blob/master/build/integration/features/sharing-v1.feature#L280

@PVince81 PVince81 force-pushed the webdav-copypermissions branch from bfaaff0 to dc6c1bf Compare July 11, 2016 10:53
@PVince81
Copy link
Contributor Author

@SergioBertolinSG ok, I added a download check. I had to put the download code in a try-catch so we can check the 404 in some cases.

| shareWith | user0 |
And As an "user0"
When User "user0" moves file "/textfile0.txt" to "/testshare/textfile0.txt"
Then the HTTP status code should be "403"
Copy link
Contributor

Choose a reason for hiding this comment

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

This when and then should be And, seems confusing otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find having a second When more readable: "When doing X, Then I see Y". In this verification case we are doing two separate things to verify two. Are you sure this isn't valid ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And Downloading file XYZ
And the HTTP status code should be 404

isn't more readable. It is confusing because you can't distinguish between which one is the action and which one the check.

Copy link
Contributor

@SergioBertolinSG SergioBertolinSG Jul 11, 2016

Choose a reason for hiding this comment

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

It should be only one when statement.

I see two choices, just check the download and not the http answer or use And statements.

(Or perhaps write another new Then which checks downloaded file and http answer, which I don't recommend now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, as you wish. I'll use the And statements then because I want to have both checks (aka asserts) and don't want to duplicate the whole test.

This gives me the feeling that either Behat is not suitable or we might be using it wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it is suitable but every test should have only one when statement. We are reusing a typical "when" which downloads the file. Perhaps we could copy it as a then statement adding the content check and just verify downloaded file.

@PVince81 PVince81 force-pushed the webdav-copypermissions branch from dc6c1bf to afea210 Compare July 11, 2016 11:35
@PVince81
Copy link
Contributor Author

@SergioBertolinSG done

@SergioBertolinSG
Copy link
Contributor

@PVince81 Sorry for the nitpicking, but I was referring to the first when and then, so making it and and when then.

@PVince81
Copy link
Contributor Author

@SergioBertolinSG I still don't get it. Can you make the change yourself ?

@SergioBertolinSG
Copy link
Contributor

@SergioBertolinSG I still don't get it. Can you make the change yourself ?

Sure.

@SergioBertolinSG
Copy link
Contributor

SergioBertolinSG commented Jul 11, 2016

Changed tests. There was a function already prepared for this case.

@PVince81
Copy link
Contributor Author

@jvillafanez @SergioBertolinSG @DeepDiver1975 @guruz second review ?

@georgehrke
Copy link
Contributor

Looks good
👍 if the tests pass

@SergioBertolinSG
Copy link
Contributor

👍

@PVince81 PVince81 merged commit ac26b8b into master Jul 12, 2016
@PVince81 PVince81 deleted the webdav-copypermissions branch July 12, 2016 06:46
@PVince81
Copy link
Contributor Author

Will prepare backports

@PVince81
Copy link
Contributor Author

stable9.1: #25448
stable9: #25449
stable8.2: #25451

@PVince81
Copy link
Contributor Author

stable8.1: #25452
stable8: #25453

Please help review all the backports!

@lock
Copy link

lock bot commented Aug 5, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants