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

Basic auth requests always set up the filesystem #31357

Open
1 task
nickvergessen opened this issue Feb 25, 2022 · 12 comments · Fixed by #36589
Open
1 task

Basic auth requests always set up the filesystem #31357

nickvergessen opened this issue Feb 25, 2022 · 12 comments · Fixed by #36589

Comments

@nickvergessen
Copy link
Member

nickvergessen commented Feb 25, 2022

ToDo

  • remove support for old chunking v1 in Desktop

If a request is authenticated and does not use cookies, it will always set up the filesystem.
The reason is the post-login gc we do:

server/lib/base.php

Lines 815 to 831 in e6d9ef2

try {
$cache = new \OC\Cache\File();
$cache->gc();
} catch (\OC\ServerNotAvailableException $e) {
// not a GC exception, pass it on
throw $e;
} catch (\OC\ForbiddenException $e) {
// filesystem blocked for this request, ignore
} catch (\Exception $e) {
// a GC exception should not prevent users from using OC,
// so log the exception
\OC::$server->getLogger()->logException($e, [
'message' => 'Exception when running cache gc.',
'level' => ILogger::WARN,
'app' => 'core',
]);
}

I think it is time to move that to a proper cron job instead. It would potentially clean up chunked upload attempts but there is no need to do this with every successful login?!

@PVince81
Copy link
Member

I think chunked upload cleanup already has a cron job: #30772

@juliusknorr
Copy link
Member

This seems to only cleanup parts from the very old chunking https://github.com/owncloud/core/wiki/spec%3A-big-file-chunking, mabye something we can get rid of then.

@tobiasKaminsky can maybe check if that is still used in any way in the clients as I found at least a code occurence for desktop: https://github.com/nextcloud/desktop/blob/4135b757b9599f95376a416761ad5cf3116411a9/src/libsync/propagateuploadv1.cpp#L115 but I'd think everything is using the new chunked upload anyways

@juliusknorr
Copy link
Member

Other than that there seems to be no usage of the \OC\Cache\File class which contains the garbage collection

@tobiasKaminsky
Copy link
Member

Discussed with @mgallien we will remove support with 3.7 and announce this in release notes.
As we have our chunked uploading mechanism this is is a decent replacement.

@juliusknorr
Copy link
Member

Is this being used as a fallback only? Would it be a problem for older clients of we drop the support in server e.g. for 26?

@mgallien
Copy link
Contributor

mgallien commented Nov 24, 2022

Is this being used as a fallback only? Would it be a problem for older clients of we drop the support in server e.g. for 26?

the new chunking algorithm has been released with 2.3.0 desktop client release from Mar 3, 2017
I think it is safe to assume clients have upgraded or we can ask them to upgrade

I mean that it should be safe to remove this deprecated code in the 26 server release

@tobiasKaminsky
Copy link
Member

@mgallien once you have removed it in Desktop, please link it here, so that we have this for reference :)

@nickvergessen
Copy link
Member Author

We can bump

$minimumSupportedDesktopVersion = $this->config->getSystemValue('minimum.supported.desktop.version', '2.0.0');
to be sure

@mgallien
Copy link
Contributor

We can bump

$minimumSupportedDesktopVersion = $this->config->getSystemValue('minimum.supported.desktop.version', '2.0.0');

to be sure

see #35425
I did it directly

@juliusknorr
Copy link
Member

Another usage is any class that injects ICache:

$this->registerService(ICache::class, function ($c) {
return new Cache\File();
});
/** @deprecated 19.0.0 */
$this->registerDeprecatedAlias('UserCache', ICache::class);

Might be harder to find all cases, but maybe we can actually switch this to a ArrayCache or local memory cache instance instead.

@juliusknorr
Copy link
Member

Reopening since #36589 was reverted for now

@juliusknorr juliusknorr reopened this Feb 21, 2023
@juliusknorr
Copy link
Member

#36787 has details about the issues that were caused

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

Successfully merging a pull request may close this issue.

6 participants