change password and new user code

[sinisaos]

Ability to change password and create a new user.

[codecov-commenter]

Codecov Report

Merging #168 (94087b9) into master (2fbdec3) will decrease coverage by 2.22%.
The diff coverage is 58.33%.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   97.54%   95.32%   -2.23%     
==========================================
  Files           3        3              
  Lines         204      214      +10     
==========================================
+ Hits          199      204       +5     
- Misses          5       10       +5     

Impacted Files	Coverage Δ
piccolo_admin/endpoints.py	95.26% <58.33%> (-2.26%)	⬇️

---

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 2fbdec3...94087b9. Read the comment docs.

[dantownsend]

One challenge we have at the moment is when creating a user, you can't set whether they're active / admin / superuser.

In example.py you've registered BaseUser with the admin, so someone could go to the list view and set these values. However, the problem is anyone could modify it (not just superusers).

I added something to PiccoloCRUD ages ago called validators:

• https://piccolo-api.readthedocs.io/en/latest/crud/piccolo_crud.html#piccolo_api.crud.endpoints.Validators

So we can stop certain users from performing certain actions on PiccoloCRUD. This might be one work around - we automatically add some extra validators for BaseUser.

Another option is to modify the register endpoint so superusers can specify the admin / active / superuser values. There's kind of a mismatch at the moment - the register endpoint was intended for people self registering, but in the admin we're registering users on their behalf. It might be better to just create a custom endpoint for superusers to register new users.

Sorry for the long comment - do you see what I'm saying?

[sinisaos]

Good point. We can hide the BaseUser table from TableNav.vue if the user is not a superuser similar to disabling user creation and this is not a problem, but I find one big problem. When we register BaseUser in create_admin, we can perform crud operations, however if for example we want to add some privilege to the user or change some other column, with row editing the password is returned as empty string (with Varchar column was everything ok, but Secret column is not displayed in admin and when editing a column in admin with Secret column or secret=True admin UI return empty string after form submit and that is a problem) and that user can no longer log in because his password is empty string.

[dantownsend]

@sinisaos Yes, that's also a problem. One solution is to not send back a value for a secret field when doing a PATCH request. Alternatively, there might just have to be some custom UI for listing / editing users, which incorporates a proper change password form.

[sinisaos]

@dantownsend We could easily fix this by setting BaseUser password column as Varchar and everything would be fine, but that's not the solution because we have to enable proper editing of any table that has columns with secret=True because it doesn't work properly at the moment either.
I’m going to try to show the secret in an edit form somehow so maybe I'll come up with something.

[sinisaos]

@dantownsend I changed my approach to this. I will completely remove the create_user endpoint from Piccolo Admin (because you are right that the register endpoint is good for the end user, but not for administrators) and with some changes in Piccolo API we will be able to create and edit a users in Piccolo Admin in its existing form via PiccoloCRUD, but with the ability to properly create hashed passwords. Only the superuser will be able to do this, and for all others User table will not be displayed at all in the admin UI. I have to write some tests, but I will do it tomorrow. I'll do PR for the Piccolo API and Piccolo Admin changes so you can see what it's all about and give your opinion

[dantownsend]

@sinisaos Thanks. I agree - I don't think we can use the register endpoint. Either we use PiccoloCRUD somehow, or build a new endpoint for this specific purpose.

[lgtm-com]

This pull request introduces 1 alert when merging 48c22fa into 2fbdec3 - view on LGTM.com

new alerts:

• 1 for Unused import

[sinisaos]

@dantownsend When you have time, create a patch for the local Piccolo API with the code from this PR, and then you can try new changes in Piccolo Admin. Hopefully this is better than the previous create_user endpoint.

[dantownsend]

@sinisaos I've added validators to show you what I'm talking about.

At the moment, the validators are pretty aggressive - they prevent non superusers from adding/editing/deleting.

[sinisaos]

@dantownsend This is great and work with latest Piccolo API PR.

and

If you have time, see the Piccolo API PR. I'm interested in your opinion. Exceptions still need to be added if that code suits you