Skip to content

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

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
Merged
juliusknorr 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

juliusknorr merged 3 commits into from
Dec 5, 2023

Conversation

@juliusknorr
Copy link
Member

@juliusknorr juliusknorr 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

@juliusknorr juliusknorr force-pushed the perf/login-with-email-token branch from b396c27 to f4668c2 Compare November 30, 2023 15:55
@juliusknorr juliusknorr requested review from a team, come-nc, icewind1991, kesselb, nfebe and nickvergessen and removed request for a team November 30, 2023 15:55
@nickvergessen nickvergessen added this to the Nextcloud 29 milestone Nov 30, 2023
@juliusknorr juliusknorr marked this pull request as ready for review November 30, 2023 19:50
@juliusknorr juliusknorr force-pushed the perf/login-with-email-token branch from 381db45 to 0ccf84b Compare November 30, 2023 19:51
@juliusknorr
Copy link
Member Author

juliusknorr commented Dec 4, 2023

@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

@kesselb kesselb Dec 4, 2023

Choose a reason for hiding this comment

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

Nice shortcut ;)

nfebe
nfebe approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@nfebe nfebe 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.

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

juliusknorr commented Dec 5, 2023

/backport to stable28

@juliusknorr
Copy link
Member Author

juliusknorr commented Dec 5, 2023

/backport to stable27

@juliusknorr
Copy link
Member Author

juliusknorr commented Dec 5, 2023

/backport to stable26

@juliusknorr
Copy link
Member Author

juliusknorr commented Dec 5, 2023

/backport to stable25

@nfebe
Copy link
Contributor

nfebe commented Dec 6, 2023

Automatic backports have failed...

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

juliusknorr commented Dec 6, 2023

/backport a3a343c,3cd1d74a815531c9b7ae25ebbf8e7c45fa566e74 to stable28

@juliusknorr
Copy link
Member Author

juliusknorr commented Dec 6, 2023

/backport a3a343c,3cd1d74a815531c9b7ae25ebbf8e7c45fa566e74 to stable27

@juliusknorr
Copy link
Member Author

juliusknorr commented Dec 6, 2023

/backport a3a343c,3cd1d74a815531c9b7ae25ebbf8e7c45fa566e74 to stable26

@juliusknorr
Copy link
Member Author

juliusknorr commented Dec 6, 2023

/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

@nfebe nfebe nfebe approved these changes

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 is a code owner automatically assigned from nextcloud/server-backend

@come-nc come-nc Awaiting requested review from come-nc come-nc is a code owner 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.

5 participants

@juliusknorr @nfebe @nickvergessen @kesselb