fix: don't set session user in getCurrentUserId - #1486
Conversation
Reverts the setSessionUser() calls added in nextcloud#1376. IApacheBackend::getCurrentUserId() is called from the first line of loginWithApache(), which guards its whole login block on the active user not already being set. Setting the session user inside getCurrentUserId() satisfies that guard before core reaches it, so the entire block gets skipped: no oc_authtoken row, no remember-me cookie, no filesystem setup, no login events. The missing token row breaks any later request that reuses the session cookie. Session::validateSession() looks up a token by session id, finds none, and calls logout(), which strips the cookie and returns a 401. Confirmed independently against master and 8.11.0-dev: bearer request then cookie-only request goes 200 then 401 with the calls in place, 200 then 200 with them removed. DAV also reaches this code through apps/dav's own handleApacheAuth() call site, confirmed 207 both before and after. Signed-off-by: mostafa <mostafakhaki00@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Stand-by before merging, I'll try to write a regression tests here. |
getCurrentUserId() runs as the first statement of OC_User::loginWithApache(), which guards its whole login block on the session user not being set yet. IUserSession::setUser() persists 'user_id', the key OC_User::getUser() reads, so setting the session user from inside getCurrentUserId() closed that guard and left the request without an oc_authtoken row. Cover the three bearer return paths: each must resolve the user id without IUserSession::setUser() being called and without 'user_id' reaching the session. setVolatileActiveUser() is deliberately not covered, it does not persist 'user_id' and so does not close the guard. Fails on the three call sites removed here, passes without them. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
|
Pushed a regression test covering the three bearer return paths: each must resolve the user id without The |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
Fixes #1452
Problem
setSessionUser(), added in #1376, is called from the first line ofgetCurrentUserId().loginWithApache()guards its whole login block on the active user not already being set:IUserSession::setUser()persistsuser_idinto the session, so by the time control reaches the guard it's already satisfied and the entire block is skipped. Nooc_authtokenrow is ever created for that session.Impact
Not just app passwords (#1452's original symptom).
Session::validateSession()looks up a token by session id on every request that reuses the session cookie; with no row to find, it callslogout(), which strips the cookie. A client that authenticates once with a bearer token and relies on the cookie for follow-up requests gets a 401 on the very next request.Confirmed independently on master and
8.11.0-dev: bearer request then cookie-only request goes200→401with the calls in place,200→200with them removed.DAV also reaches this code, through its own
handleApacheAuth()call site inapps/dav/lib/Connector/Sabre/Auth.php, separate fromOC::handleLogin(). Confirmed207both before and after this change.Why a revert
getCurrentUserId()is called beforeloginWithApache()'s guard, and there's no hook in between — nothing the app does insidegetCurrentUserId()can both satisfy the guard and leave a token row behind, since satisfying the guard is what skips the code that creates the row. A "complete the login inside the app" approach was considered and dropped: it only works by keeping the guard closed on purpose, which means maintaining a parallel login path here that has to keep tracking core's.The
setupFS/keygen cost this avoided is real, but it's a separate, filed issue — after this revert, every bearer request without a session cookie re-pays it, same as before #1376.Testing
composer lint && composer cs:check && composer psalmclean, no baseline changes.No unit test added —
tests/unit/has no existing coverage ofBackend.php, and this is only observable end-to-end (bearer request → cookie reuse), which needs a running server, not a unit fixture.