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

api update. added password recovery parameter for password change wit… #7736

Closed

Conversation

akalevich
Copy link

Api update. added password recovery parameter for password change with enabled encryption. #7735

@MorrisJobke MorrisJobke added the 3. to review Waiting for reviews label Jan 9, 2018
@MorrisJobke MorrisJobke modified the milestones: Nextcloud 13, Nextcloud 14 Jan 9, 2018
@nickvergessen
Copy link
Member

Looks good, can you adjust the unit tests so they work again and add one for this new case?

@nickvergessen
Copy link
Member

Can you please sign your commits? See https://github.com/nextcloud/server/blob/master/CONTRIBUTING.md#sign-your-work for more information.

You can do this manually for now:

git commit --amend -s
git push --force origin <your branch>

@codecov
Copy link

codecov bot commented Jan 11, 2018

Codecov Report

Merging #7736 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master    #7736     +/-   ##
===========================================
+ Coverage     51.12%   51.22%   +0.1%     
+ Complexity    25724    24970    -754     
===========================================
  Files          1574     1607     +33     
  Lines         88510    95011   +6501     
  Branches          0     1376   +1376     
===========================================
+ Hits          45252    48673   +3421     
- Misses        43258    46338   +3080
Impacted Files Coverage Δ Complexity Δ
...rovisioning_api/lib/Controller/UsersController.php 81.76% <100%> (+9.69%) 122 <32> (-14) ⬇️
apps/admin_audit/lib/Actions/GroupManagement.php 0% <0%> (-100%) 4% <0%> (ø)
...are/Security/Exceptions/AppNotEnabledException.php 0% <0%> (-100%) 1% <0%> (ø)
apps/workflowengine/lib/Check/RequestUserAgent.php 0% <0%> (-96.43%) 10% <0%> (-4%)
lib/private/Preview/GeneratorHelper.php 0% <0%> (-80%) 5% <0%> (ø)
apps/dav/lib/AppInfo/PluginManager.php 0% <0%> (-71.93%) 31% <0%> (ø)
apps/admin_audit/lib/Actions/AppManagement.php 0% <0%> (-66.67%) 3% <0%> (ø)
apps/files_sharing/lib/Helper.php 20.35% <0%> (-58.96%) 38% <0%> (+27%)
apps/comments/lib/Activity/Listener.php 23.52% <0%> (-56.87%) 10% <0%> (ø)
apps/admin_audit/lib/Actions/Action.php 0% <0%> (-56%) 7% <0%> (ø)
... and 734 more

…h enabled encryption

Signed-off-by: akalevich <r_alex_b@tut.by>
Signed-off-by: akalevich <r_alex_b@tut.by>
@akalevich
Copy link
Author

akalevich commented Jan 11, 2018

Done. But, if this method can use the user on himself and admin on any user, I have to write two tests? On change password without recovery one test written. I did the same.

@MorrisJobke
Copy link
Member

@akalevich Sorry that this somehow slipped through :/ We added type hinting to this method which causes the merge conflict. Do you want to resolve this or should we take over and resolve it on our side?

@MorrisJobke
Copy link
Member

@skjnldsv Didn't you added this lately?

@skjnldsv
Copy link
Member

@schiessle :)
We decided to block password change if the master key is disabled and encryption enabled, can you confirm?

@akalevich
Copy link
Author

akalevich commented Apr 11, 2018

@MorrisJobke

Do you want to resolve this or should we take over and resolve it on our side?

Is it still topical?

@MorrisJobke
Copy link
Member

Do you want to resolve this or should we take over and resolve it on our side?
Is it still topical?

Sure we could take over this one if you wish.

@MorrisJobke
Copy link
Member

We decided to block password change if the master key is disabled and encryption enabled, can you confirm?

Then this can be closed.

@akalevich
Copy link
Author

@MorrisJobke Done

@skjnldsv
Copy link
Member

@akalevich sorry, but for security reasons, we decided to simply block password change if the master key is disabled and encryption enabled. Therefore this pull request won't be merged :(

@skjnldsv skjnldsv closed this May 26, 2018
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants