refactor: modernize session management and token validation#1390
refactor: modernize session management and token validation#1390
Conversation
solracsf
force-pushed
the
refactorUserBackend
branch
2 times, most recently
from
44990c5 to
e1b4226
Compare
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
Thanks a lot for refactoring this. Could you add a doc block comment for all the new methods with a global description? Especially if they change the overall behaviour of the backend.
|
@julien-nc will adress all your comments at once ASAP. |
|
@solracsf There is no rush 😁 |
solracsf
force-pushed
the
refactorUserBackend
branch
from
e1b4226 to
c392b41
Compare
solracsf
force-pushed
the
refactorUserBackend
branch
from
c392b41 to
7c966c9
Compare
|
|
||
| try { | ||
| // copy skeleton | ||
| $userFolder = Server::get(IRootFolder::class)->getUserFolder($userId); | ||
| \OC_Util::copySkeleton($userId, $userFolder); | ||
| } catch (NotPermittedException $ex) { | ||
| // read only uses | ||
| } catch (NotFoundException|NotPermittedException $e) { | ||
| $this->logger->warning('Could not set up user folder on first login', ['exception' => $e]); | ||
| } |
There was a problem hiding this comment.
This means the login does not crash anymore if something went wrong in copySkeleton. I'm not sure we want that. Wdyt?
There was a problem hiding this comment.
The old code had getUserFolder() outside the try block, catching only NotPermittedException from copySkeleton. So a NotFoundException thrown by getUserFolder() itself would have propagated up uncaught, potentially crashing first login.
The refactor moved getUserFolder() inside the try block and added NotFoundException to the catch. This means two failure modes are now silently handled that previously weren't:
getUserFolder()throwingNotFoundException(storage not found)copySkeletonthrowingNotFoundException
Whether this is the right call, it depends: if you want the login to proceed even if home folder setup fails, then this is correct and a net improvement. If you want setup failures to be hard errors that block login, you'd need to let them propagate. Given that NotPermittedException was already silently swallowed for "read only uses", continuing on NotFoundException seems consistent.
Your call here.
There was a problem hiding this comment.
If there are storage-related errors, it's bad anyway.
|
Just in case you haven't seen: there are conflicts with main. Again: no rush, just a notification. |
solracsf
force-pushed
the
refactorUserBackend
branch
2 times, most recently
from
30e2188 to
6680b7a
Compare
- Extract bearer token parsing, OIDC config reading, validator instantiation, NC provider validation, and user resolution into dedicated private methods to reduce cognitive complexity of getCurrentUserId() - Replace the first-match-wins loop with findUniqueTokenMatch(), which requires unambiguous validation across all providers: a token accepted by more than one (provider, userId) pair is rejected with a warning - Add countUsers() via a direct COUNT(*) query instead of fetching all UIDs into memory - Introduce SESSION_USER_DATA, SESSION_PASSWORD_CONFIRM, and PASSWORD_CONFIRM_TTL constants to avoid magic strings/numbers - Fix first-login setup: move getUserFolder() inside the try/catch so a NotFoundException from storage setup no longer aborts login - Keep $existingUser null when a user is freshly created via getOrCreate() so provisionUser() is not passed the wrong identity when UNIQUE_UID or PROVIDER_BASED_ID cause the stored ID to differ from the token sub - Add doc blocks to all new private methods, with extended descriptions for methods that change observable backend behaviour Signed-off-by: Git'Fellow <[email protected]>
solracsf
force-pushed
the
refactorUserBackend
branch
from
6680b7a to
2315fff
Compare
|
Done 💯 |
2 participants
This PR is a comprehensive audit and refactor, resolving bugs and performance problems identified through iterative code review. Refactor user session management and token validation logic for improved clarity and functionality. Introduce constants for session data and enhance error handling.