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.

Sign up for GitHub

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

Jump to bottom

perf: Use more performant way to obtain and check the email as a login name with token login #41927

Merged
merged 3 commits into from
Dec 5, 2023
Merged

perf: Use more performant way to obtain and check the email as a login name with token login #41927

merged 3 commits into from
Dec 5, 2023

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Nov 30, 2023 •
edited
Loading

UserManager::getByEmail is slow on large userbases so we should avoid it where possible.

  • When logging in with a token we first try to verify against the login name passed but then do a second attempt using the email address. As we already know the user id that we need to look up the email for we can use the stored uid from the token and compare the email instead of searching by mail
  • Revert change to always use an sql cast that was a behavioral change from 8bbdd9e between 24 and 25
  • Add a partial index to the configvalue to ensure that most email checks can use an indexed query -> Moved to perf: Add partial index on configvalue of preferences table #42022
    KEY `tmp_configvalue_index` (`configvalue`(80)),
    • OCI doesn't like that index cannot create index on expression with datatype LOB

TODO:

  • Come up with a good approach for adding a test case to avoid future regressions in performance

@juliushaertl juliushaertl requested review from nickvergessen, kesselb, a team, icewind1991, Fenn-CS and come-nc and removed request for a team November 30, 2023 15:55
@nickvergessen nickvergessen added this to the Nextcloud 29 milestone Nov 30, 2023
@juliushaertl juliushaertl marked this pull request as ready for review November 30, 2023 19:50
@juliushaertl
perf: Use more performant way to obtain and check the email as a logi…
a3a343c 
…n name with token login

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
fix: Only apply cast if needed when searching users for value
3cd1d74 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
perf: Add partial index on configvalue of preferences table
0ccf84b 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

@nickvergessen As discussed via chat, I split out the additional index and moved it to #42022 to not forget about that.

kesselb
kesselb reviewed Dec 4, 2023
View reviewed changes
lib/private/User/Session.php
if (!(\count($users) === 1 && $this->login($users[0]->getUID(), $password))) {

if ($isTokenPassword) {
$dbToken = $this->tokenProvider->getToken($password);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice shortcut ;)

Fenn-CS
Fenn-CS approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@Fenn-CS Fenn-CS left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, so good.

By the way, I think it's helpful to attach the point explanations in the pull request description in the various commit bodies.

@juliushaertl juliushaertl merged commit 6c52242 into master Dec 5, 2023
48 of 50 checks passed
@juliushaertl juliushaertl deleted the perf/login-with-email-token branch December 5, 2023 10:11
@juliushaertl
Copy link
Member Author

/backport to stable28

@juliushaertl
Copy link
Member Author

/backport to stable27

@juliushaertl
Copy link
Member Author

/backport to stable26

@juliushaertl
Copy link
Member Author

/backport to stable25

@Fenn-CS
Copy link
Contributor

Fenn-CS commented Dec 6, 2023

Automatic backports have failed...

@juliushaertl juliushaertl mentioned this pull request Dec 6, 2023
Merged
@juliushaertl
Copy link
Member Author

/backport a3a343c,3cd1d74a815531c9b7ae25ebbf8e7c45fa566e74 to stable28

@juliushaertl
Copy link
Member Author

/backport a3a343c,3cd1d74a815531c9b7ae25ebbf8e7c45fa566e74 to stable27

@juliushaertl
Copy link
Member Author

/backport a3a343c,3cd1d74a815531c9b7ae25ebbf8e7c45fa566e74 to stable26

@juliushaertl
Copy link
Member Author

/backport a3a343c,3cd1d74a815531c9b7ae25ebbf8e7c45fa566e74 to stable25

This was referenced Jan 9, 2024
Merged
Merged
Merged
Merged
@blizzz blizzz mentioned this pull request Mar 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kesselb kesselb kesselb left review comments

@nickvergessen nickvergessen nickvergessen approved these changes

@Fenn-CS Fenn-CS Fenn-CS approved these changes

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 was automatically assigned from nextcloud/server-backend

@come-nc come-nc Awaiting requested review from come-nc come-nc was automatically assigned from nextcloud/server-backend

Assignees
No one assigned
Labels
2. developing Work in progress bug feature: authentication performance 🚀
Projects
None yet
Milestone
Nextcloud 29
Development

Successfully merging this pull request may close these issues.
4 participants
@juliushaertl @Fenn-CS @nickvergessen @kesselb