Simplify the check if admin can change password based on encryption status#23906
Merged
MorrisJobke merged 1 commit intomasterfrom Nov 5, 2020
Merged
Conversation
Member
|
🤖 beep boop beep 🤖 Here are the logs for the failed build: Status of 35056: failuresqliteShow full logmariadb10.1-php7.3Show full logmariadb10.4-php7.4Show full logmysql8.0-php7.4
mysql5.6-php7.3Show full logpostgres9-php7.3Show full logpostgres11-php7.4Show full log |
PVince81
reviewed
Nov 5, 2020
PVince81
approved these changes
Nov 5, 2020
Member
PVince81
left a comment
There was a problem hiding this comment.
Right... these twisted conditions were confusing.
👍
juliusknorr
approved these changes
Nov 5, 2020
…tatus Found by Psalm: ``` /home/runner/work/server/server/apps/settings/lib/Controller/UsersController.php:324:8:error - RedundantCondition: Type true for $isEncryptionModuleLoaded is never falsy ``` Signed-off-by: Morris Jobke <hey@morrisjobke.de>
44d1b04 to
a223f60
Compare
Member
Author
|
Okay - I was wrong on the initial simplification. There was the case that encryption is disabled but a module is still loaded which prohibits password change (and was also like that documented as well as in the unit tests). So I will keep the logic like that and only removed the redundant condition that Psalm found. It was this one (first and last line are together independent from $canChangePassword = ($isEncryptionEnabled && $isEncryptionModuleLoaded && $noUserSpecificEncryptionKeys)
|| (!$isEncryptionEnabled && !$isEncryptionModuleLoaded)
|| (!$isEncryptionEnabled && $isEncryptionModuleLoaded && $noUserSpecificEncryptionKeys); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Simplify the check if admin can change password based on encryption status
Found by Psalm:
Old and wrong initial comment
Was initially added in #23857 by @schiessle
Found by Psalm:
This is basically a simplification of the logical statement there. The PHPDoc comment "encryption is enabled, encryption module is loaded and it uses per-user keys" was code-wise not true and therefore is removed here.
This may needs a verification by @schiessle because comment and code were not aligned.
Let me explain some simplifications:
If you look at the old code:
If you check the first and the last line you notice that it's exactly the same except that
$isEncryptionEnabledis negated. Therefore they can be combined into$isEncryptionModuleLoaded && $noUserSpecificEncryptionKeys.For the try block the first part would depend on
$noUserSpecificEncryptionKeysand the second part would befalse.And for the catch block the first part of the statement would be
falseand the second part only depends on the!$isEncryptionEnabled.This then would lead to:
And that latter part is what Psalm found out. 🤯 🤯 🤯 🤯 🤯 🤯
All in all it boils down to be able to allow password changes when encryption is not enabled or check for user specific encryption keys and if they are not available or the exception is thrown the password can be changed as well.
This is what this quite complex change here does.
I will dedicate this PR description to @danxuliu and @PVince81 because they write quite often those exhaustive descriptions for their PRs. 🙏