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

Possible bug when editing user's username property #368

Closed
tcompa opened this issue Dec 12, 2023 · 6 comments
Closed

Possible bug when editing user's username property #368

tcompa opened this issue Dec 12, 2023 · 6 comments
Labels
to be discussed Not ready to implement

Comments

@tcompa
Copy link
Collaborator

tcompa commented Dec 12, 2023

(found with @marcodeltufo)

To reproduce:

  1. Go to localhost:5173/admin/users
  2. Edit a user who already has a given value for username
  3. Remove the value of username and click "save".
  4. No error is observed
  5. The user still has the same username.

Note that the request body of the PATCH API call is

{
"id":1,
"email":"[email protected]",
"is_active":true,
"is_superuser":true,
"is_verified":false,
"slurm_user":"test01",
"cache_dir":"/tmp \\asd"
}

but it does not include username.


Is something wrong here? Or did we exclude it on purpose due to some weird request-body-validation rule in fractal-server? (cc @ychiucco)

@tcompa tcompa added the bug Something isn't working label Dec 12, 2023
@tcompa
Copy link
Collaborator Author

tcompa commented Dec 12, 2023

Addendum: when instead of removing the current value we replace it with a different string, the PATCH goes through and the change takes place.
Therefore I guess we introduced some fractal-web logic to avoid sendind a request with username=null to the fractal-server PATCH endpoint, most likely because it would not be accepted.

@tcompa
Copy link
Collaborator Author

tcompa commented Dec 12, 2023

My suggestions:

  1. It is a bit weird that this property is removed from the payload without any warning/error message, in fractal-web.
  2. This username change looks like something that an admin should be able to do. If they can modify a user's username, then they should also be able to remove it. This likely requires a (minor) change in fractal-server. Note: I may be contradicting past me, with this remark.. if so, I'm sorry for that ;)

@ychiucco
Copy link
Collaborator

Currently we do not accept None as UserUpdate.username, but we can make it possible by changing this line
https://github.com/fractal-analytics-platform/fractal-server/blob/673e1c95b7de9b6f0a54f97eaf080ea80abdd569/fractal_server/app/schemas/user.py#L52C1-L52C8
into
_username = validator("username", allow_reuse=True)(valstr("username", accept_none=True))

@zonia3000
Copy link
Collaborator

I confirm that in the frontend we are removing the null values. If I allow them I receive:

String attribute 'username' cannot be None

@tcompa
Copy link
Collaborator Author

tcompa commented Jul 16, 2024

@tcompa tcompa added to be discussed Not ready to implement and removed bug Something isn't working labels Aug 26, 2024
@tcompa
Copy link
Collaborator Author

tcompa commented Sep 6, 2024

Closing, as the use of username should be superseded by the future task access-control configuration (based on either task-user relationship or task-user_group relationship, and not on string matching for specific columns).
Specific backend issue(s) TBD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to be discussed Not ready to implement
Projects
Development

No branches or pull requests

3 participants