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.
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! |
There was a problem hiding this comment.
Okay, I think I figured out what's bothering me about this PR w.r.t. the janky auth stuff. The functions need to change.
As designed, they're kind of left over from the REST style, but a major goal of the RPC change is to leave that stuff behind. Instead of the current setup for patch, we should emulate the password change functions: One for 'self', and another for 'other'. Crucially, we don't need a user_id passed to patch our own user: That is leftover thinking from the REST endpoint. So, we should have a patch_self function that doesn't take a user_id, and which any user can access, and a patch_other function that does take a user_id, and which only a superuser can access. Also, the replace_own function shouldn't take a user_id at all! This is completely unneeded, and only serves to force us to make that awkward auth check to make sure that a user can only modify their own password. Finally, I think these functions should take primitive arguments rather than a SettableUserInfo object. The reason is that it makes it a bit clearer what the function is actually allowed to modify, and it's a bit safer. The patch_self function shouldn't even take a is_superuser parameter.
The front end change should be pretty easy for the patch stuff, since the function that calls is already aware of whether the user is modifying themselves or another. You just need to call the appropriate RPC function at that point.
Sorry to keep going around on this stuff, but I think it's pretty important to avoid adding any ad-hoc auth pieces if we can avoid it.
Bonus idea: Given that we'd be splitting out patch_self and patch_other, we should be able to wrap up the password change logic into the same function, since that's actually just a patch. The only reason we had it separate in the first place is because of the self vs. other distinction. This is optional, but it would simplify the module a bit.
|
@seancolsen I've made some changes for patching users as suggested by Brent, in 7b31e81, can you please review them? |
There was a problem hiding this comment.
Okay, thanks for all your work on this. I'm happy with it once @seancolsen signs off.
There was a problem hiding this comment.
Looks good! I pushed 57857cf with some additional cleanup to simplify the types.
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