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.

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

fix(auth): Fix logging in with email and app password #42971

Merged

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Jan 19, 2024

Summary

Logging in with email and app password is only allowed if the token was created in a web session authenticated with the email. That's because Nextcloud enforces the login name to match when logging in with an app password.

\OCA\DAV\Connector\Sabre\Auth::validateUserPass calls \OC\User\Session::logClientIn, which tries to log in with \OC\User\Session::login, which validates the token. Because of the login name mismatch login "fails". Yet \OC\User\Session::logClientIn continues the email login fallback. Since #42651 there is a shortcut to not do a second login. But the return value indicates a successful login that never happened.

Tested

  • Log in with username and password -> works as expected
  • Log in with username and app password generated with username login -> works as expected
  • Log in with email and password -> works as expected
  • Log in with email and app password generated with username login -> fails gracefully as expected
  • Log in with email and app password generated with email login -> fails gracefully as expected

Checklist

@ChristophWurst
Copy link
Member Author

Log in with email and app password generated with email login

Weirdly enough that is not possible anymore. If I log in using email+password on web, any generated app token receives my user's UID as login name, not the email address. This is another regression.

@ChristophWurst
Copy link
Member Author

If I log in using email+password on web, any generated app token receives my user's UID as login name, not the email address. This is another regression.

That has even been the case since #15365. I guess sessions with email login names are not possible unless it's ldap.

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 19, 2024
@ChristophWurst ChristophWurst marked this pull request as ready for review January 19, 2024 19:01
@ChristophWurst ChristophWurst changed the title fix(auth): Fix logging in with email, password and login name mismatch fix(auth): Fix logging in with email and app password Jan 19, 2024
@ChristophWurst ChristophWurst changed the title fix(auth): Fix logging in with email and app password fix(auth): Fix loging in with email and app password Jan 19, 2024
@ChristophWurst ChristophWurst changed the title fix(auth): Fix loging in with email and app password fix(auth): Fix logging in with email and app password Jan 22, 2024
@ChristophWurst
Copy link
Member Author

/backport to stable28

@ChristophWurst
Copy link
Member Author

/backport to stable27

@ChristophWurst
Copy link
Member Author

/backport to stable26

@ChristophWurst
Copy link
Member Author

/backport to stable25

@blizzz blizzz mentioned this pull request Jan 22, 2024
@blizzz
Copy link
Member

blizzz commented Jan 22, 2024

If I log in using email+password on web, any generated app token receives my user's UID as login name, not the email address. This is another regression.

That has even been the case since #15365. I guess sessions with email login names are not possible unless it's ldap.

IIRC we have a questionable design where mail login is attempted independent of the user backends. The LDAP backend passes on the provided login name, and by configuration it could be compared against a user id, email, both, or other attributes as well.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

LGTM and login using the email still works for me, but didn't test with app passwords.

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 22, 2024
@ChristophWurst
Copy link
Member Author

mysql killed after 1h. rest passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: authentication regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: DAV login with email address: Call to a member function getUID() on null
4 participants