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.

Sign up for GitHub

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

Jump to bottom

Prevent LOCK/UNLOCK methods from public endpoint #34351

Merged
merged 2 commits into from
Feb 1, 2019
Merged

Prevent LOCK/UNLOCK methods from public endpoint #34351

merged 2 commits into from
Feb 1, 2019

Conversation

PVince81
Copy link
Contributor

Description

Prevent LOCK/UNLOCK methods from public endpoint

Related Issue

Fixes #34347

Motivation and Context

How Has This Been Tested?

  • unit test

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@PVince81 PVince81 added this to the development milestone Jan 31, 2019
@PVince81 PVince81 self-assigned this Jan 31, 2019
@PVince81 PVince81 mentioned this pull request Jan 31, 2019
Closed
@PVince81
Copy link
Contributor Author

might need additional acceptance test changes
cc @individual-it

phil-davis
phil-davis approved these changes Jan 31, 2019
View reviewed changes
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

Code looks good. See what acceptance tests now fail and adjust those for the changed behavior.

@codecov
Copy link

codecov bot commented Jan 31, 2019

Codecov Report

Merging #34351 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34351      +/-   ##
============================================
+ Coverage     64.76%   64.76%   +<.01%     
- Complexity    18368    18370       +2     
============================================
  Files          1199     1199              
  Lines         69551    69555       +4     
  Branches       1281     1281              
============================================
+ Hits          45046    45050       +4     
  Misses        24132    24132              
  Partials        373      373
Flag Coverage Δ Complexity Δ
#javascript 53.09% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.76% <100%> (-0.35%) 18370 <1> (+2)
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Files/FileLocksBackend.php 91.66% <100%> (+0.41%) 26 <1> (+2) ⬆️
apps/files_external/lib/Lib/Storage/Swift.php 66.16% <0%> (ø) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f82ad1...70cac25. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 31, 2019 •
edited
Loading

Codecov Report

Merging #34351 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34351      +/-   ##
============================================
+ Coverage     64.76%   64.76%   +<.01%     
- Complexity    18368    18370       +2     
============================================
  Files          1199     1199              
  Lines         69551    69555       +4     
  Branches       1281     1281              
============================================
+ Hits          45046    45050       +4     
  Misses        24132    24132              
  Partials        373      373
Flag Coverage Δ Complexity Δ
#javascript 53.09% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.12% <100%> (ø) 18370 <1> (+2) ⬆️
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Files/FileLocksBackend.php 91.66% <100%> (+0.41%) 26 <1> (+2) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f82ad1...7a66731. Read the comment docs.

@PVince81
Copy link
Contributor Author

I've adjusted the acceptance tests, let's see.

I've removed most of the tests by public. @individual-it sorry for losing your hard work on all these cases. If we ever bring this back we can restore from this commit.

Only two tests remain to check that public cannot lock or unlock.
For the lock case, we get 403.
For unlock, I needed an owner with a lock, and the error is 409 instead of 403.

@PVince81
Copy link
Contributor Author

Closes #34347
Closes #34304
Closes #34302

@PVince81 PVince81 force-pushed the lock-prevent-public branch from 70cac25 to d57b6fd Compare January 31, 2019 21:34
@PVince81
Copy link
Contributor Author

removed even more obsolete tests, squashed

@PVince81 PVince81 mentioned this pull request Jan 31, 2019
Open
@PVince81
Copy link
Contributor Author

added a note in #34222 (comment) so we can remember that there are tests we can recover once we address federation with locking

Remove most public link locking tests
7a66731 
And check that public cannot lock or unlock at all
@PVince81 PVince81 force-pushed the lock-prevent-public branch from d57b6fd to 7a66731 Compare February 1, 2019 06:12
@PVince81
Copy link
Contributor Author

PVince81 commented Feb 1, 2019

annnnd one last deletion, squashed.

CI should pass now, then @individual-it @phil-davis can review the test changes

individual-it
individual-it approved these changes Feb 1, 2019
View reviewed changes
Copy link
Member

@individual-it individual-it left a comment

Choose a reason for hiding this comment

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

@PVince81 reviewed the deleted tests, they look good ;-)

@PVince81 PVince81 merged commit 895f07d into master Feb 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the lock-prevent-public branch February 1, 2019 06:48
This was referenced Feb 1, 2019
Merged
Closed
Closed
@phil-davis
Copy link
Contributor

Backport stable10 #34355

@lock lock bot locked as resolved and limited conversation to collaborators Feb 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@phil-davis phil-davis phil-davis approved these changes

@individual-it individual-it individual-it approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

Assignees

@PVince81 PVince81

Labels
3 - To Review
Projects
None yet
Milestone
development
Development

Successfully merging this pull request may close these issues.

Public webdav should not be able to lock read-only public links
3 participants
@PVince81 @phil-davis @individual-it