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 direct password edit #1038

Merged
merged 3 commits into from
Nov 16, 2019
Merged

Fix direct password edit #1038

merged 3 commits into from
Nov 16, 2019

Conversation

rwos
Copy link
Contributor

@rwos rwos commented Oct 1, 2019

Bug: editing a users password does not work, it always ends up as invalid because "the passwords don't match".

This fixes that by making the name of the password field flexible:

  • 'password' on the user creation form
  • 'value' on the edit password form (since that goes through admin's UserController::updateField)

this replaces #1034
see also #1030

I now see that the whole thing could've also been fixed by calling the password field "value" (and reverting the above PR), but that looks a bit weird IMHO. But let me know what you think!

by making the name of the password field flexible:
* 'password' on the user creation form
* 'value' on the edit password form
@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

Merging #1038 into hotfix will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             hotfix   #1038      +/-   ##
===========================================
+ Coverage     66.66%   66.7%   +0.03%     
  Complexity     1931    1931              
===========================================
  Files           162     162              
  Lines          6737    6745       +8     
===========================================
+ Hits           4491    4499       +8     
  Misses         2246    2246
Impacted Files Coverage Δ Complexity Δ
.../sprinkles/admin/src/Controller/UserController.php 99.6% <100%> (ø) 112 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18bc0c3...5bc185b. Read the comment docs.

@amosfolz
Copy link
Contributor

amosfolz commented Oct 2, 2019

@rwos Have you tested to make sure this is compatible with updating existing users password?

@rwos
Copy link
Contributor Author

rwos commented Oct 2, 2019

@amosfolz that's exactly the thing that this fixes. But I would appreciate someone other than me checking this again with v4.3.1 (setting a manual password on user creation works, updating a user's password is broken) and this PR (both work). I'm reasonably sure this works, but I was reasonably sure in #1030 too :-)

@n0use
Copy link

n0use commented Oct 2, 2019

My original comment said this only fixed CHANGING a password on 4.3.1 - this turned out to be my own fault, a sprinkle I had written had an old copy of the schema/requests/user/create.yaml file. When I got this updated (the offending difference was field: password vs field: value) everything password related, creating users with passwords or changing existing users passwords, works with this PR. Thanks!

I just tried this on 4.3.1 and it does fix CHANGING a users password, which is good, but I get the same "Your password and confirmation password must match." error when trying to CREATE a user with a password set.

@amosfolz
Copy link
Contributor

amosfolz commented Oct 2, 2019

I think this still needs some work. For example, validation messages appear not to match anymore:

image

@lcharette
Copy link
Member

#1034 (comment)

The "single controller method accepting any field" thing could be phased out for password if this is too much of an issue.

@rwos
Copy link
Contributor Author

rwos commented Oct 7, 2019

Alright - validation should be fixed now, and it now also uses the site.password.length config everywhere (edit password + user create), as it should.

@lcharette
Copy link
Member

So... Which one between this (#1038) and #1034 actually fix this issue?

@lcharette lcharette self-assigned this Oct 14, 2019
@lcharette lcharette added the confirmed bug Something isn't working label Oct 14, 2019
@lcharette lcharette added this to the 4.3.x milestone Oct 14, 2019
@amosfolz
Copy link
Contributor

This is the one I tested - everything worked besides the validation message. If that is fixed, this should be merged and the other closed.

@lcharette lcharette merged commit 8952fda into userfrosting:hotfix Nov 16, 2019
lcharette added a commit that referenced this pull request Nov 17, 2019
Removed `value` as the default field name when editing a single field. 
The field name should now be the proper name. For example, editing 
`flag_enable`, the data should be `flag_enabled => 1` instead of `value 
=> 1`
@lcharette
Copy link
Member

I've merged this, and with #1034, made other improvement in 29bfdd4. Should be good to go now.

I change the behavior of UserController::updateField. Instead of using a common value key for all fields, the key is now the field name itself.

For example, when updating flag_enable, the data should be flag_enabled => 1 instead of value => 1. So now edit-password.yaml will have the proper password value in it.

Could introduce a minor Breaking change if someone uses that class in it's own sprinkle. Will add a note in the changelog.

@lcharette lcharette removed this from the 4.5.x milestone Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants