-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Do not setup a session when not required on API requests #28311
Conversation
Some integration test failures I've not seen so far. Will look into those:
They all seem related to the attempt to use the session id:
|
b33554f
to
77aee15
Compare
|
Hopefully soon gets merged. I want to get rid of that thousands of session files. |
/rebase |
bdaa565
to
16052db
Compare
this could also break tools like https://github.com/owncloud/pyocclient which also rely on the session, I imagine that there might be more out there so this PR would need to be treated as "breaking change" which we cannot backport or if backport, have a config switch with a different default in previous versions for those setups where the admin knows what kind of clients are being used and maybe such switch could be turned on for the integration tests to pass, for now @juliushaertl
|
16052db
to
0471bbd
Compare
0471bbd
to
55dd770
Compare
Tests:
|
The failures are all in auth.feature and happen in the preconditions:
|
it looks like the tests like https://github.com/nextcloud/server/blob/enh/http-auth-session/build/integration/features/auth.feature#L19 were expecting basic auth to create a session when done on the web UI endpoints like "index.php/apps/files" as the subsequent test is relying on said session but in this PR the session is not created any more in that case, not sure if we should keep this or if we should filter the session removal to DAV and OCS only ? @juliushaertl depending on the latter decision we'll need to rework the tests to reflect the expected behavior |
Note: the test issues so far were not at all related to the missing session, it was only that some test code was written in the wrong way but did work because a session happened to be there (ex: missing cookie jar usage) |
aa1fe70
to
01d5aca
Compare
Breaks integration tests on notifications app:
Problem is: server/lib/private/Authentication/LoginCredentials/Store.php Lines 100 to 101 in 215aef3
|
Maybe we can check the controller and see if it uses server/core/Controller/AppPasswordController.php Lines 77 to 94 in 215aef3
But then again it shows exactly why this PR is kind of dangerous. IIRC this endpoint is used by the android apps when they copy credentials from each other?
Also used in iOS |
This comment was marked as outdated.
This comment was marked as outdated.
Talk is also completely red CI. Maybe we first limit this to webdav endpoints? OCS stuff needs a lot more looking first: |
@@ -412,6 +412,15 @@ private static function printUpgradePage(\OC\SystemConfig $systemConfig) { | |||
} | |||
|
|||
public static function initSession() { | |||
$request = self::$server->getRequest(); | |||
$isDavRequest = strpos($request->getRequestUri(), '/remote.php/dav') === 0 || strpos($request->getRequestUri(), '/remote.php/webdav') === 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about public webdav ? basic auth is used there with share token as user and share password as password
🏓 for reviews now for 26 |
Is this something that affects clients? |
It should not change anything, I did a smoke test at the last performance hackweek with desktop, but smoke testing with iOS/Android would still be highly appreciated :) |
Smoke test on Android works. |
d0b0706
to
d4e994e
Compare
Rebased. Still looking for another review 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good otherwise
If basic auth is used on WebDAV endpoints, we will not setup a session by default but instead set a test cookie. Clients which handle session cookies properly will send back the cookie then on the second request and a session will be initialized which can be resued for authentication. Signed-off-by: Julius Härtl <jus@bitgrid.net>
d4e994e
to
6abb373
Compare
Checker failure unrelated, fix is in #35855 |
This would resolve #7628 as currently every API request to webdav/OCS that is using HTTP auth is generating a PHP session even though it is never used afterwards as any follow-up request is sending the auth headers again and therefore just creating another session. This reduces the amount of sessions being created and that would need to be kept by the php session handler which might otherwise grow extensively on large setups with logs of API calls happening.
As we already use a wrapper for the session returning early in the session setup will just result in the memory session implementation being used as a fallback
server/lib/private/Server.php
Line 516 in 7179002
Ideally the check would be performed when the request is handled and the login checks are performed is handled, but the risk of breaking something when setting up the session that late in the execution flow seems much higher.
Fix #27603