Users RPC transition #4026
Users RPC transition #4026
Conversation
|
@mathemancer I am planning on adding tests and api documentation in a separate PR, since this one is getting too big. |
There was a problem hiding this comment.
Ooo how fun! I love seeing a PR with some front end work too from you @Anish9901 🙂
This is mostly great, but I pushed b34300e to fix a small issue...
In WritableUsersStore, the request property was previously a CancellablePromise because that's was was returned from api.users.list(). The fact that it's a CancellablePromise is pretty much the whole reason we're storing at as a class variable here — so that we can cancel it later. But the new api.users.list() function doesn't return a CancellablePromise. You don't get that until you call run() on it. So I changed some things to make sure that request stayed as a CancellablePromise and then I added the cancellation call back in which you removed.
(As a side note, this .run() calling was my design and after working with it, I don't actually like this design, but that's another story — it's just to say that I think my poor design is part of what led you astray here.)
I've reviewed the front end code and done a small amount of playing with the app to verify that things work okay. Looks good to me!
There was a problem hiding this comment.
Some changes to request. See specific comments. The overall points are:
- Try to use the built-in
django-modern-rpcauth system for theis_selfcheck. - Avoid spreading "auth-like" checks across multiple files.
Question: Was doing this full-stack the reason that you needed to do all endpoints at once? I could see how that would happen if there are dependencies in the front end between the old REST endpoints. I'd generally prefer reducing scope on a PR like this by reducing the number of endpoints transitioned at once, rather than skipping tests and docs.
| @rpc_method(name='users.delete') | ||
| @http_basic_auth_superuser_required | ||
| @handle_rpc_exceptions | ||
| def delete(*, user_id: int) -> None: | ||
| delete_user(user_id) |
There was a problem hiding this comment.
Innnteresting. This removes the ability of a user to delete their own account from the back end, but I note that the front end doesn't allow that for some reason anyway. Was this change in the back end behavior intentional, or just fortuitous?
There was a problem hiding this comment.
After looking at the issue, I must have noticed that the front end wasn't using the 'delete own' functionality when I wrote it. Or, it was just lucky.
There was a problem hiding this comment.
This is intentional, we only allow superusers to delete accounts, including their own apparently, although not through the UI.
mathesar/rpc/users.py
Outdated
| **kwargs | ||
| ) -> UserInfo: | ||
| user = kwargs.get(REQUEST_KEY).user | ||
| if not (user.id == user_id or user.is_superuser): |
There was a problem hiding this comment.
Did you try to use the set_authentication_predicate decorator for this? I think it would be cleaner and more reusable to try to use that, or set up a custom auth decorator for the "is self or superuser" case.
There was a problem hiding this comment.
It would also ensure that if the upstream package changes the response for failure of these decorators, that this function doesn't behave differently.
There was a problem hiding this comment.
I think it is next to impossible to use set_authentication_predicate for testing is_self because, for is_self, we would require user_id which is a parameter that we pass through the request. The way django_modern_rpc is set up, the permission checks set up through set_authentication_predicate happen before the RPC function's args and kwargs are populated for RPC function execution.
So, even something like this doesn't work:
@wraps(func)
def wrap(*args, **kwargs):
user_id = kwargs.get('user_id')
wrapper = auth.set_authentication_predicate(http_basic_auth_check_user, [user_is_self_or_superuser, user_id])
return wrapper(func)(*args, **kwargs)
I also tried getting the user_id using the request, but request.POST results in None when printed inside http_basic_auth_check_user().
I finally settled on writing a custom decorator but without using set_authentication_predicate.
mathesar/rpc/users.py
Outdated
| user = kwargs.get(REQUEST_KEY).user | ||
| if not user.id == user_id: |
There was a problem hiding this comment.
Same here. Try to use a custom auth decorator. You can just add one that guarantees that there's a user.id in the request that equals the user_id. Then, we have an added benefit of having a failure if a developer for some reason names the parameter something unexpected (i.e., something other than user_id)
| def update_user_info(user_id, user_info, requesting_user): | ||
| if not requesting_user.is_superuser: | ||
| user_info.pop("is_superuser", None) | ||
| User.objects.filter(id=user_id).update( |
There was a problem hiding this comment.
I'm suspicious of having an auth-related step (making sure a non-superuser can't elevate their privileges) in this function, rather than the RPC layer function. Did you consider putting it there? We don't want to have to look in multiple files to validate privilege logic in this case, unless there's a really good reason.
Also, it seems like User.objects.get be better here. Why did you go with the filter function? Specifically, I think we'd want to throw an Exception if there's no matching object. Maybe I'm missing something.
Finally, can't you just explode the user_info dict for this update call? E.g.,
User.objects.get(id=user_id).update(**user_info)
There was a problem hiding this comment.
I'm suspicious of having an auth-related step (making sure a non-superuser can't elevate their privileges) in this function, rather than the RPC layer function. Did you consider putting it there?
I did consider putting it there but thought that it would be better to add all the logic related to updating user in a single function instead of splitting it, this way we won't forget to check whether the requesting_user is a superuser, if we decided to use the update_user_info function elsewhere. But oh well..., we could also use the RPC functions directly instead. I've updated it as per your request.
Also, it seems like User.objects.get be better here. Why did you go with the filter function? Specifically, I think we'd want to throw an Exception if there's no matching object. Maybe I'm missing something.
Django advises against doing this: https://docs.djangoproject.com/en/4.2/ref/models/querysets/#django.db.models.query.QuerySet.update
The return of the update_user_info() function is get_user() which will raise an error anyway if the user doesn't exist.
Finally, can't you just explode the
user_infodict for this update call? E.g.,User.objects.get(id=user_id).update(**user_info)
We could explode the user_info dict but I decided to avoid it since someone could potentially add additional arguments in that dict to alter other fields of User model. e.g. fields like is_active, is_staff, password_change_needed etc.
We can't use update on a User object or any object we receive with get, we can only use it with filter. Yep, I know... kinda stupid. But I think its because update performs a SQL update instead of loading the result in memory first, contrary to the way get works.
mathesar/utils/users.py
Outdated
| def change_password(user_id, old_password, new_password): | ||
| user = get_user(user_id) | ||
| if not user.check_password(old_password): | ||
| raise Exception('Old password is not correct') | ||
| user.set_password(new_password) |
There was a problem hiding this comment.
Same here. I don't think this layer should be authenticating against the user's old password. That should be the RPC layer (unless I'm missing something).
Nope, They seemed to be small and less complex to implement thats why I did it in one shot.
I'll keep that in mind next time. |
|
Thanks for the detailed explanation @seancolsen, that was really helpful!
I wouldn't call it bad design, when I first started reading the frontend code for this PR, admittedly my brain was turning into mush looking at the amount of interfaces there were. Mind you the only experience I had with JS before working on this PR is writing Nonetheless, you can expect more frontend contributions from me in the future! |
Fixes #3993
Fixes #4000
Fixes #4001
Fixes #4002
Fixes #4003
Fixes #4004
Fixes #4005
Fixes #4006
Checklist
Update index.md).developbranch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin