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

fix can change password check in case of encryption is enabled #13172

Merged
merged 1 commit into from
Jan 4, 2019
Merged

fix can change password check in case of encryption is enabled #13172

merged 1 commit into from
Jan 4, 2019

Conversation

schiessle
Copy link
Member

Admin should not be able to change password when:

  • if an encryption module is loaded and it uses per-user keys
  • if encryption is enabled but no encryption modules are loaded

Admin should be able to change the password when:

  • no encryption module is loaded and encryption is disabled
  • encryption module is loaded but it doesn't require per user keys

fix #11212

@schiessle schiessle mentioned this pull request Dec 20, 2018
Closed
skjnldsv
skjnldsv reviewed Dec 20, 2018
View reviewed changes
settings/Controller/UsersController.php Outdated
$noUserSpecificEncryptionKeys = true;
$isEncryptionModuleLoaded = false;
}

// If masterKey enabled, then you can change password. This is to avoid data loss!
Copy link
Member

Choose a reason for hiding this comment

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

Are the comments still accurate?

Copy link
Member Author

Choose a reason for hiding this comment

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

more or less yes, but let me update the comment and add some more details

Copy link
Member Author

Choose a reason for hiding this comment

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

done 🙂

@schiessle
Copy link
Member Author

This makes it easier for people to disable encryption completely and be able to change passwords again as a admin. Also for example, if they just enabled the "default encryption module" app by accident but never really turned on server side encryption.

Of course this carries one (small) risk:
You could imagine a installation where encryption was enabled and files are encrypted with per user keys. Now the admin disables both SSE and the default encryption modules, changes passwords and now realize that they have to enable the default encryption module again to let the user still decrypt old encrypted files. In this case the user can still adjust the private key password in the personal settings if they remember the old password so maybe this is acceptable.

@schiessle schiessle added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 20, 2018
@schiessle
Copy link
Member Author

schiessle commented Dec 20, 2018 •
edited
Loading

I will move this check to a separate function so that we can write some tests for it... done ✔️

@schiessle schiessle added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 20, 2018
@schiessle
fix can change password check in case of encryption is enabled
4b3308b 
Admin should _not_ be able to change password when:
- if an encryption module is loaded and it uses per-user keys
- if encryption is enabled but no encryption modules are loaded

Admin should be able to change the password when:
- no encryption module is loaded and encryption is disabled
- encryption module is loaded but it doesn't require per user keys

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
@MorrisJobke MorrisJobke mentioned this pull request Jan 4, 2019
Merged
3 tasks
@rullzer rullzer merged commit fe3d8ff into master Jan 4, 2019
@rullzer rullzer deleted the fix-can-change-password-check branch January 4, 2019 08:53
@rullzer
Copy link
Member

rullzer commented Jan 4, 2019

/backport to stable15

@rullzer
Copy link
Member

rullzer commented Jan 4, 2019

/backport to stable14

@backportbot-nextcloud
Copy link

backport to stable15 in #13349

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 4, 2019
Merged
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 4, 2019
Merged
@backportbot-nextcloud
Copy link

backport to stable14 in #13350

@laurentiu2
Copy link

Does not work for me. I updated from 13.0.8 to 14.0.5 after this was merged and the issue is still there.

In 13.0.8 I have the master key enabled and as admin I can change the user's password. In 14.0.5 the field for master key is gone.

@jkoopmann
Copy link

Is it possible this came back with 15.0.7? I have not touched my encryption settings for years and now with 15.0.7 (maybe started with 15.0.x) I suddenly get the warning/error I cannot change passwords:

./occ encryption:status

  • enabled: false
  • defaultModule: OC_DEFAULT_MODULE

@Jockelchen
Copy link

Jockelchen commented Jul 1, 2019 •
edited
Loading

I've got a more disturbing question but related.
I cant change my users Password since upgrading to nextcloud 16 (16.0.1.1). I can change the Masterkey, but it doesn't get recognized.

occ encryption:status

  • enabled: true
  • defaultModule: OC_DEFAULT_MODULE

how can i fix this? I was asked do change a users password soon.

@the-sane the-sane mentioned this pull request Aug 16, 2019
Closed
@Jockelchen
Copy link

So, how can i fix this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rullzer rullzer rullzer approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug feature: encryption (server-side) feature: users and groups
Projects
None yet
Milestone
Nextcloud 16
Development

Successfully merging this pull request may close these issues.

Upgrade 13->14: master key disabled
7 participants
@schiessle @rullzer @laurentiu2 @jkoopmann @Jockelchen @skjnldsv @MorrisJobke