Added validation for the user update route/UI #646

BenjaminCharmes · 2024-03-13T18:04:22Z

Linked to #640

Frontend:

Add error message and disabled submitting if display_name or contact_email isn't validated

Backend:

For more 'security', also add validator on the user model and in the save_user method so a user can't be updated with unvalidated data even if the form is already disabled.

ml-evs

Some minor immediate comments. Reviewing out of order as @jdbocarsly is going to also take a look at #627 first (which we will merge before this one).

Looks good so far, we can just discuss the right approach.

Although it seems a bit gross to have validation on both the client and server, I think it makes sense in this case since it really is easy to validate. One thing that might be missing is how the UI deals with a server-side validation, so you might want to test disabling the client side validation (e.g., making the display name max length on the client 151 and then providing a 151 char display name to the server to make sure that a nice error is returned).

pydatalab/pydatalab/routes/v0_1/users.py

pydatalab/pydatalab/models/people.py

pydatalab/pydatalab/routes/v0_1/users.py

ml-evs

This looks good to me, thanks Ben! Just one comment about the display name (below) and the suggestion that we should add a simple test that the validator does what we expect.

pydatalab/pydatalab/models/people.py

webapp/src/components/EditAccountSettingsModal.vue

ml-evs

Hi @BenjaminCharmes, this is looking good, just a few minor comments (of things we can think about in future PRs) and one that I think should be addressed now.

pydatalab/pydatalab/models/people.py

pydatalab/pydatalab/routes/v0_1/users.py

pydatalab/pydatalab/models/people.py

webapp/src/components/EditAccountSettingsModal.vue

ml-evs

See my comment here: https://github.com/the-grey-group/datalab/pull/646/files#r1539219700

I will remove some of the validation I asked for as we can never cover all cases, otherwise looks good!

ml-evs · 2024-03-26T13:26:47Z

pydatalab/pydatalab/routes/v0_1/users.py

+        if contact_email or contact_email in (None, ""):
+            if contact_email == "":
+                contact_email = None
+            update["contact_email"] = EmailStr(contact_email)


None is not a valid EmailStr so we'll have to move that around a bit

ml-evs · 2024-03-26T14:31:09Z

Okay, running into a few issues (that it looks like I ran into last time I did this since an issue I opened myself 2 years ago was the first result when I googled...)

Basically the custom types defined here either work in the pydantic model OR in the the type itself, but never in both. I've just updated them so that they work in both (by making them both constrained string types in pydantic and calling their validation methods on __new__).

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

…splay_name Co-authored-by: Matthew Evans <[email protected]>

Removed unnecessary validator [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

Update front and back user's display_name validation [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

… of pydantic Display name updates in schema

Add test for display name with Chiense characters

ml-evs

Okay, running into a few issues (that it looks like I ran into last time I did this since an issue I opened myself 2 years ago was the first result when I googled...)

Basically the custom types defined here either work in the pydantic model OR in the the type itself, but never in both. I've just updated them so that they work in both (by making them both constrained string types in pydantic and calling their validation methods on new).

Have just tidied these up with my additional commits, as well as adding some integration tests to check that things behave. Happy to merge this now but will try to preserve the commit history so you can see what I changed @BenjaminCharmes!

Nice work and thanks again!

jdbocarsly

Looks good! Tiny comment on one of the error messages.

Also, I'm noticing that when you are done with this modal and close it, the LoginDetails menu is still open. I think it would probably be more user friendly if it were closed, which you could do by changing line 28 in LoginDetails.vue to:

@click="editAccountSettingIsOpen = true; isUserDropdownVisible = false;"

webapp/src/components/EditAccountSettingsModal.vue

Co-authored-by: Josh Bocarsly <[email protected]>

BenjaminCharmes requested review from ml-evs and jdbocarsly as code owners March 13, 2024 18:04

ml-evs reviewed Mar 13, 2024

View reviewed changes

BenjaminCharmes linked an issue Mar 18, 2024 that may be closed by this pull request

Validate display names and contact emails #640

Closed

ml-evs mentioned this pull request Mar 20, 2024

Created component for editing account settings #627

Merged

BenjaminCharmes force-pushed the bc/user-validators branch from 1997bca to d1d41a6 Compare March 20, 2024 14:59

ml-evs changed the title ~~[WIP] Added validation for the model user~~ Added validation for the model user Mar 20, 2024

ml-evs self-requested a review March 20, 2024 16:39

ml-evs changed the title ~~Added validation for the model user~~ Added validation for the user update route/UI Mar 20, 2024

ml-evs reviewed Mar 20, 2024

View reviewed changes

pydatalab/pydatalab/models/people.py Outdated Show resolved Hide resolved

webapp/src/components/EditAccountSettingsModal.vue Outdated Show resolved Hide resolved

ml-evs reviewed Mar 25, 2024

View reviewed changes

ml-evs reviewed Mar 26, 2024

View reviewed changes

BenjaminCharmes and others added 14 commits March 26, 2024 15:09

Added validators in people model and validators in frontend edit modal

7aca341

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

Added validation in save_user

19e7719

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

Update models/people.py to simplify updating the max_len value for di…

38fb419

…splay_name Co-authored-by: Matthew Evans <[email protected]>

Removed unnecessary validator

05b16ff

Removed unnecessary validator [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

Update DisplayName validator with new custom type

5ff5264

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

Readd change

8fe0e18

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

Update front and back user's display_name validation

a09cddd

Update front and back user's display_name validation [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

Add test for user's model display_name validation

574f964

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

Update test for bad display_name

bc00f81

Update user's validation

3ce076d

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

Tweak EmailStr and DisplayName models to work both inside and outside…

c06a8c1

… of pydantic Display name updates in schema

Tweak user route to group errors and allow for easier testing

e89d583

Add test for display name with Chiense characters

Add tests for save user endpoint

62aaa6b

Renable client-side validation but weaken constraints

1bc621d

ml-evs force-pushed the bc/user-validators branch from 8dff18d to 1bc621d Compare March 26, 2024 15:09

ml-evs self-requested a review March 26, 2024 15:11

ml-evs previously approved these changes Mar 26, 2024

View reviewed changes

jdbocarsly requested changes Mar 26, 2024

View reviewed changes

webapp/src/components/EditAccountSettingsModal.vue Outdated Show resolved Hide resolved

ml-evs dismissed their stale review via ab6386f March 26, 2024 16:00

Add suggestions from code review

7699896

Co-authored-by: Josh Bocarsly <[email protected]>

ml-evs force-pushed the bc/user-validators branch from ab6386f to 7699896 Compare March 26, 2024 16:01

ml-evs merged commit 40aaf96 into datalab-org:main Mar 26, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added validation for the user update route/UI #646

Added validation for the user update route/UI #646

BenjaminCharmes commented Mar 13, 2024

ml-evs left a comment

ml-evs left a comment

ml-evs left a comment

ml-evs left a comment

ml-evs Mar 26, 2024

ml-evs commented Mar 26, 2024

ml-evs left a comment

jdbocarsly left a comment

Added validation for the user update route/UI #646

Added validation for the user update route/UI #646

Conversation

BenjaminCharmes commented Mar 13, 2024

ml-evs left a comment

Choose a reason for hiding this comment

ml-evs left a comment

Choose a reason for hiding this comment

ml-evs left a comment

Choose a reason for hiding this comment

ml-evs left a comment

Choose a reason for hiding this comment

ml-evs Mar 26, 2024

Choose a reason for hiding this comment

ml-evs commented Mar 26, 2024

ml-evs left a comment

Choose a reason for hiding this comment

jdbocarsly left a comment

Choose a reason for hiding this comment