[full-ci] Revert loader logic in ChunkLocationProvider.php by pako81 · Pull Request #40700 · owncloud/core

pako81 · 2023-03-24T19:08:47Z

Description

Revert loader logic in ChunkLocationProvider.php

Related Issue

https://github.com/owncloud/firewall/issues/742

Motivation and Context

File Firewall acceptance tests were failing after merging #40693 as we changed the position of the loader in the constructor. Not sure why this is happening but reverting the change seems to make the tests to pass.

How Has This Been Tested?

Ran File Firewall acceptance tests locally and make sure that following scenarios pass (they were previously failing):

webUILimitUpload/limituploadbasedonsize.feature:29
webUILimitUpload/limituploadbasedonsize.feature:37
webUILimitFileAccess/limitfileaccessbasedonrequesttype.feature:13

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:

update-docs · 2023-03-24T19:08:54Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

ownclouders · 2023-03-24T19:17:17Z

💥 Acceptance tests pipeline webUISharingExt2-git-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/38132/155

ownclouders · 2023-03-24T19:17:17Z

💥 Acceptance tests pipeline webUIRestrictSharing-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/38130/132

jvillafanez · 2023-03-27T07:57:48Z

I think we'll have to investigate what's happening there. Accordingly to the constructor in https://github.com/owncloud/core/blob/master/lib/private/Files/Mount/MountPoint.php#L79 , if the loader is null, we use an "empty" storage factory instead of whatever storage factory is prepared. In particular, some wrappers could be set in place in the factory we want to inject, but they won't be present in the newly created one (if the loader is null).

If that's the case, we should remove the loader parameter in this PR because it's unused.

We probably should also investigate why the code is dependent on a specific loader; the wrappers around the storages shouldn't matter I think.

phil-davis · 2023-03-27T11:15:46Z

Yes, changing this code here does help the firewall tests to all pass. But actually the change here is removing the passing of "loader" to the MountPoint constructor. It gets passed as an entry in the $arguments parameter and gets ignored. So if this is the "fix" then we should completely remove $loader from being passed at all.

As @jvillafanez says, this needs investigation to understand what the code used to do, and what it should do. (And to try and provide a solution that is reasonably upward-compatible for apps like firewall - so that we don't have to require specific versions of firewall to go with specific releases of core)

Fix changelog typo #40511

Bumps [phpunit/phpunit](https://github.com/sebastianbergmann/phpunit) from 9.6.5 to 9.6.6. - [Release notes](https://github.com/sebastianbergmann/phpunit/releases) - [Changelog](https://github.com/sebastianbergmann/phpunit/blob/main/ChangeLog-9.6.md) - [Commits](sebastianbergmann/phpunit@9.6.5...9.6.6) --- updated-dependencies: - dependency-name: phpunit/phpunit dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>

…hpunit-9.6.6 Bump phpunit/phpunit from 9.6.5 to 9.6.6

…#40702) * occ encryption:encrypt-all should not auto-enable user-key encryption * add changelog and tests * Update changelog/unreleased/40702 Co-authored-by: Phil Davis <phil@jankaritech.com> --------- Co-authored-by: Phil Davis <phil@jankaritech.com>

* Add rewrite base to generated .htaccess rules If htaccess.RewriteBase is set we need to prepend it to the generated rules. * Fix potential double slash and empty rewriteBase if rewriteBase is "/" * use preg_quote() for proper regex escaping. * Update lib/private/Setup.php --------- Co-authored-by: Juergen Weigert <jnweiger@gmail.com>

* prevent 507 Insufficient Storage on 32bit systems * add changelog * improve if-else logic * retrigger CI

* fix permission bits when enforcing rw links * adapt test to match the code. * list all cases that the admin config for password enforces refers to --------- Co-authored-by: Piotr Mrówczyński <piotr@owncloud.com>

pako81 · 2023-03-29T08:30:55Z

When running the tests locally, in both cases - with and without loader in ChunkLocationProvider.php - following is logged (which is expected):

{"reqId":"zG36QILFddhbJ6cEzTpW","level":4,"time":"2023-03-27T20:05:59+00:00","remoteAddr":"128.140.6.151","user":"admin","app":"webdav","method":"PUT","url":"\/owncloud\/remote.php\/webdav\/filespecificSize.txt-chunking-3098-3-0","message":"Exception: HTTP\/1.1 403 Access to this resource has been forbidden by a file firewall rule.: {\"Exception\":\"OCA\\\\DAV\\\\Connector\\\\Sabre\\\\Exception\\\\Forbidden\",\"Message\":\"Access to this resource has been forbidden by a file firewall rule.\",\"Code\":0,\"Trace\":\"
#0 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(1098): OCA\\\\DAV\\\\Connector\\\\Sabre\\\\Directory->createFile()\\n
#1 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/CorePlugin.php(504): Sabre\\\\DAV\\\\Server->createFile()\\n
#2 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/event\\\/lib\\\/WildcardEmitterTrait.php(89): Sabre\\\\DAV\\\\CorePlugin->httpPut()\\n
#3 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(472): Sabre\\\\DAV\\\\Server->emit()\\n
#4 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(253): Sabre\\\\DAV\\\\Server->invokeMethod()\\n
#5 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(321): Sabre\\\\DAV\\\\Server->start()\\n
#6 \\\/var\\\/www\\\/html\\\/owncloud\\\/apps\\\/dav\\\/appinfo\\\/v1\\\/webdav.php(67): Sabre\\\\DAV\\\\Server->exec()\\n
#7 \\\/var\\\/www\\\/html\\\/owncloud\\\/remote.php(165): require_once('\\\/var\\\/www\\\/html\\\/o...')\\n
#8 {main}\",\"File\":\"\\\/var\\\/www\\\/html\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/Connector\\\/Sabre\\\/Directory.php\",\"Line\":185}"}

In addition, with the loader in place (which makes the firewall tests to fail) this is also logged:

**{"reqId":"ikmhcKdsCxI2FXrxI1Ok","level":4,"time":"2023-03-27T20:06:00+00:00","remoteAddr":"128.140.6.151","user":"admin","app":"webdav","method":"PUT","url":"\/owncloud\/remote.php\/dav\/uploads\/admin\/chunking-7148\/0","message":"Exception: Access to this resource has been forbidden by a file firewall rule.: {\"Exception\":\"OCP\\\\Files\\\\ForbiddenException\",\"Message\":\"Access to this resource has been forbidden by a file firewall rule.\",\"Code\":0,\"Trace\":\"
#0 \\\/var\\\/www\\\/html\\\/owncloud\\\/apps\\\/firewall\\\/lib\\\/storagewrapper.php(91): OCA\\\\Firewall\\\\Firewall->checkRulesForFiles()\\n
#1 \\\/var\\\/www\\\/html\\\/owncloud\\\/apps\\\/firewall\\\/lib\\\/storagewrapper.php(356): OCA\\\\Firewall\\\\StorageWrapper->checkFirewall()\\n
#2 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/private\\\/Files\\\/View.php(726): OCA\\\\Firewall\\\\StorageWrapper->fopen()\\n
#3 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/public\\\/Events\\\/EventEmitterTrait.php(50): OC\\\\Files\\\\View->OC\\\\Files\\\\{closure}(*** sensitive parameters replaced ***)\\n
#4 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/private\\\/Files\\\/View.php(754): OC\\\\Files\\\\View->emittingCall()\\n
#5 \\\/var\\\/www\\\/html\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/Connector\\\/Sabre\\\/Directory.php(527): OC\\\\Files\\\\View->file_put_contents()\\n
#6 \\\/var\\\/www\\\/html\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/Upload\\\/UploadFolder.php(37): OCA\\\\DAV\\\\Connector\\\\Sabre\\\\Directory->createFileDirectly()\\n
#7 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(1098): OCA\\\\DAV\\\\Upload\\\\UploadFolder->createFile()\\n
#8 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/CorePlugin.php(504): Sabre\\\\DAV\\\\Server->createFile()\\n
#9 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/event\\\/lib\\\/WildcardEmitterTrait.php(89): Sabre\\\\DAV\\\\CorePlugin->httpPut()\\n
#10 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(472): Sabre\\\\DAV\\\\Server->emit()\\n
#11 \\\/var\\\/www\\\/html\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(253): Sabre\\\\DAV\\\\Server->invokeMethod()\\n
#12 \\\/var\\\/www\\\/html\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/Server.php(348): Sabre\\\\DAV\\\\Server->start()\\n
#13 \\\/var\\\/www\\\/html\\\/owncloud\\\/apps\\\/dav\\\/appinfo\\\/v2\\\/remote.php(31): OCA\\\\DAV\\\\Server->exec()\\n
#14 \\\/var\\\/www\\\/html\\\/owncloud\\\/remote.php(165): require_once('\\\/var\\\/www\\\/html\\\/o...')\\n
#15 {main}\",\"File\":\"\\\/var\\\/www\\\/html\\\/owncloud\\\/apps\\\/firewall\\\/lib\\\/firewall.php\",\"Line\":194}"}

jvillafanez · 2023-03-29T09:18:54Z

I don't know if those errors are ok because there is a firewall rule being correctly applied, or the errors shouldn't happen...

* changelog for #40697 * update changelog --------- Co-authored-by: Pasquale Tripodi <ptripodi@owncloud.com>

…40712)

…x the path with a / (#40677)

…40717) * Add missing changelog for public link password enfocing. * skip CI --------- Co-authored-by: Pasquale Tripodi <ptripodi@owncloud.com>

jnweiger

Let us merge this for 10.12.1
I'd like to see a full CI run. Please rebase and merge.

Please open a separate issue for further investigation of the $loader wrapper logic.

…re into loader_chunklocationprovider

CLAassistant · 2023-04-03T11:00:51Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
5 out of 7 committers have signed the CLA.

✅ IljaN
✅ mrow4a
✅ phil-davis
✅ DeepDiver1975
✅ jnweiger
❌ Pasquale Tripodi
❌ ownclouders

Pasquale Tripodi seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

sonarqubecloud · 2023-04-03T11:25:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

jnweiger · 2023-04-03T12:19:56Z

My rebase attempt failed. The history here is now a mess.
Clean restart with identical code change at #40719

revert loader logic in ChunkLocationProvider.php

e714f96

pako81 added the 3 - To Review label Mar 24, 2023

pako81 added this to the development milestone Mar 24, 2023

pako81 requested review from jvillafanez and phil-davis March 24, 2023 19:08

pako81 self-assigned this Mar 24, 2023

Pasquale Tripodi and others added 3 commits March 24, 2023 20:32

retrigger CI

5eecd99

[tx] updated from transifex

794521f

Fix changelog typo #40511

2d3317e

phil-davis and others added 12 commits March 27, 2023 17:20

Merge pull request #40704 from ho4ho/patch-1

7636a6c

Fix changelog typo #40511

Automated changelog update [skip ci]

d727631

[tx] updated from transifex

455119e

Merge pull request #40707 from owncloud/dependabot/composer/phpunit/p…

8afb5c0

…hpunit-9.6.6 Bump phpunit/phpunit from 9.6.5 to 9.6.6

Automated changelog update [skip ci]

e92de0f

Prevent 507 Insufficient Storage on 32-bit systems (#40709)

b831feb

* prevent 507 Insufficient Storage on 32bit systems * add changelog * improve if-else logic * retrigger CI

Automated changelog update [skip ci]

038747c

remove loader

626cfdc

fix permission bits when enforcing rw links (#40701)

aabe1ec

* fix permission bits when enforcing rw links * adapt test to match the code. * list all cases that the admin config for password enforces refers to --------- Co-authored-by: Piotr Mrówczyński <piotr@owncloud.com>

IljaN and others added 2 commits March 29, 2023 16:12

[skip-ci] changelog for #40697 (#40711)

124fe43

* changelog for #40697 * update changelog --------- Co-authored-by: Pasquale Tripodi <ptripodi@owncloud.com>

Automated changelog update [skip ci]

144e5e2

ownclouders and others added 6 commits March 30, 2023 01:12

[tx] updated from transifex

da14163

fix: remove php occ encryption:select-encryption-type from drone (#…

4915ada

…40712)

[tx] updated from transifex

d3dd378

fix: in case root on CacheJail is an empty string we should not prefi…

6e5a4cf

…x the path with a / (#40677)

[skip-ci] Add missing changelog for public link password enforcing (#…

ccb2bd8

…40717) * Add missing changelog for public link password enfocing. * skip CI --------- Co-authored-by: Pasquale Tripodi <ptripodi@owncloud.com>

Automated changelog update [skip ci]

38c2cd7

jnweiger self-requested a review April 3, 2023 10:13

jnweiger changed the title ~~Revert loader logic in ChunkLocationProvider.php~~ [full-ci] Revert loader logic in ChunkLocationProvider.php Apr 3, 2023

jnweiger approved these changes Apr 3, 2023

View reviewed changes

Pasquale Tripodi and others added 4 commits April 3, 2023 12:59

revert loader logic in ChunkLocationProvider.php

30a61ca

retrigger CI

0898908

remove loader

f56b6f2

Merge branch 'loader_chunklocationprovider' of github.com:owncloud/co…

5dc5f1f

…re into loader_chunklocationprovider

pako81 mentioned this pull request Apr 3, 2023

[full-ci] Remove loader in ChunkLocationProvider.php #40719

Merged

11 tasks

jnweiger closed this Apr 3, 2023

jnweiger mentioned this pull request Apr 3, 2023

Testplan server 10.12.1 #40722

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[full-ci] Revert loader logic in ChunkLocationProvider.php#40700

[full-ci] Revert loader logic in ChunkLocationProvider.php#40700
pako81 wants to merge 28 commits intomasterfrom
loader_chunklocationprovider

pako81 commented Mar 24, 2023 •

edited

Loading

Uh oh!

update-docs bot commented Mar 24, 2023

Uh oh!

ownclouders commented Mar 24, 2023 •

edited

Loading

Uh oh!

ownclouders commented Mar 24, 2023

Uh oh!

jvillafanez commented Mar 27, 2023

Uh oh!

phil-davis commented Mar 27, 2023

Uh oh!

pako81 commented Mar 29, 2023 •

edited

Loading

Uh oh!

jvillafanez commented Mar 29, 2023

Uh oh!

jnweiger left a comment

Uh oh!

CLAassistant commented Apr 3, 2023 •

edited

Loading

Uh oh!

sonarqubecloud bot commented Apr 3, 2023

Uh oh!

jnweiger commented Apr 3, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Conversation

pako81 commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

update-docs bot commented Mar 24, 2023

Uh oh!

ownclouders commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ownclouders commented Mar 24, 2023

Uh oh!

jvillafanez commented Mar 27, 2023

Uh oh!

phil-davis commented Mar 27, 2023

Uh oh!

pako81 commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvillafanez commented Mar 29, 2023

Uh oh!

jnweiger left a comment

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Apr 3, 2023

Uh oh!

jnweiger commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

pako81 commented Mar 24, 2023 •

edited

Loading

ownclouders commented Mar 24, 2023 •

edited

Loading

pako81 commented Mar 29, 2023 •

edited

Loading

CLAassistant commented Apr 3, 2023 •

edited

Loading

jnweiger commented Apr 3, 2023 •

edited

Loading