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

occ user:add-app-password should also work without the users login password #26563

Closed
schiessle opened this issue Apr 14, 2021 · 6 comments · Fixed by #31687
Closed

occ user:add-app-password should also work without the users login password #26563

schiessle opened this issue Apr 14, 2021 · 6 comments · Fixed by #31687
Assignees
Labels
3. to review Waiting for reviews enhancement feature: authentication feature: occ papercut Annoying recurring issue with possibly simple fix.

Comments

@schiessle
Copy link
Member

The occ command user:add-app-password should work without the users login password.

If a admin needs to create a app password for a user for whatever reasons (e.g. in migration szenarios) it is quite unlikely that they know the login password from the users. The provisioning API and the graphical user management allow the admin to change the users password without knowing the old one. Why should the user:add-app-password be more strict?

Second, in case of SSO no user has a login password on Nextcloud. All passwords are handled by the IDP. The current behavior of the occ command makes it completely useless in any SSO environment.

Therefore I would suggest to remove the password input/check or at least make it optional.

@schiessle schiessle added enhancement 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Apr 14, 2021
@wiswedel
Copy link
Contributor

Regardless of what you enter as a password, it's not validated anyway. The generated app password works - even if the password is wrong.
ref: https://github.com/nextcloud/server/blob/master/core/Command/User/AddAppPassword.php#L118

So the password check can as well be dropped.

@solracsf solracsf changed the title occ user:add-app-password should also work without the users login password occ user:add-app-password should also work without the users login password Apr 19, 2021
@rullzer
Copy link
Member

rullzer commented Apr 21, 2021

The password is there because we validate it if the backend supports it. So it is validated on actual use.
Same for SMB etc.

But sure feel free to shoot in a PR to make it optional.

@wiswedel
Copy link
Contributor

The password is there because we validate it if the backend supports it. So it is validated on actual use.

That's the thing: we don't.

Steps to reproduce

  1. Have a local user Alice with password Bob123
  2. Test the password with a PROPFIND Dav API call authenticating with -u Alice:Bob123 --> works
  3. Test a wrong password with a PROPFIND Dav API call authenticating with -u Alice:Bob1234 --> [...] Username or password was incorrect [...] --> works as intended
  4. Run occ user:add-app-password Alice
  5. Prompt asks for password: Type asdfasdfasdf --> output=token
  6. Run Call from 2. but replace Bob123 with token from 5.

Expected behavior

  • Token gets rejected

Actual behavior

  • Token gets accepted as app password --> no validation of the entered password happening

@szaimen
Copy link
Contributor

szaimen commented Jul 2, 2021

So the generated token works although the provided password was wrong? If yes, we should probably drop all password related parts of the code. If the generated token doesn't work, we should probably validate the password correctly, no?

@ghost
Copy link

ghost commented Aug 1, 2021

This issue has been automatically marked as stale because it has not had recent activity and seems to be missing some essential information. It will be closed if no further activity occurs. Thank you for your contributions.

@ghost ghost added the stale Ticket or PR with no recent activity label Aug 1, 2021
@yoyoyonas
Copy link

So the generated token works although the provided password was wrong?

That's correct. Tested on NC 21.0.3.

@ghost ghost removed the stale Ticket or PR with no recent activity label Aug 6, 2021
@szaimen szaimen added 1. to develop Accepted and waiting to be taken care of feature: occ good first issue Small tasks with clear documentation about how and in which place you need to fix things in. papercut Annoying recurring issue with possibly simple fix. feature: authentication and removed needs info 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Aug 8, 2021
@PVince81 PVince81 removed the good first issue Small tasks with clear documentation about how and in which place you need to fix things in. label Feb 14, 2022
@ChristophWurst ChristophWurst self-assigned this Mar 23, 2022
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels Mar 23, 2022
@ChristophWurst ChristophWurst moved this to 🏗️ In progress in 💌 📅 👥 Groupware team Mar 23, 2022
Repository owner moved this from 🏗️ In progress to ☑️ Done in 💌 📅 👥 Groupware team Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement feature: authentication feature: occ papercut Annoying recurring issue with possibly simple fix.
Projects
Development

Successfully merging a pull request may close this issue.

7 participants