[#759] scan and apply user_zone properly when igroupadmin creates a user #760
Conversation
|
need to write a test wherein we create an entry in the catalog for a remote-zone user, then add and remove that user from a group. |
irods/manager/user_manager.py
Outdated
| _user_name = user_name + ("" if not user_zone else f"#{user_zone}") | ||
| else: | ||
| _user_name = user_name | ||
| user_name, user_zone = user_name.split("#") |
There was a problem hiding this comment.
Two more cases which we might want to consider / test...
- What should happen if
create_with_passwordis called like this?
groupadmin.users.create_with_password("newUser#tempZone", "newPassword", user_zone="otherZone")- Or this?
groupadmin.users.create_with_password("newUser#tempZone#otherZone", "newPassword")For reference, iadmin mkuser returns a USER_INVALID_USERNAME_FORMAT error when doing something like (2). I don't actually know if that's coming from the client or server side.
There was a problem hiding this comment.
Neither of those cases make sense to me. Can you explain what the goal of those are?
There was a problem hiding this comment.
Is the user attempting to create a remote user in the local zone?
There was a problem hiding this comment.
I think he's just writing a pathological test to check the behavior of the system.
There was a problem hiding this comment.
Does the iRODS server accept case 1?!
There was a problem hiding this comment.
That makes sense. It isn't clear to me how much is client responsibility and how much should rest with the server, though. We could, in the client, test both user and zone for being properly formatted but I have a feeling that should rest more with the server, rather than installing a redundant and possibly incorrect such identifier filtering operation in the client code
That's fair. I'm interested in testing the parameters available in this function to the extent that they are the responsibility of this client. We don't need to test the server's behavior, just that the inputs to the iRODS API call are what we want/expect for any given set of inputs to this function.
We can discuss further offline to reach an understanding and then we can move this along.
There was a problem hiding this comment.
After some discussion, we decided that create_with_password should reject user_name parameter values which contain the zone name delimited by an octothorpe ("bad"). Here's a complete set of cases to demonstrate the requirements:
# tempZone is local; otherZone is remote; fictionZone does not exist
groupadmin.users.create_with_password("newUser", "newPassword") # 1. okay - use tempZone
groupadmin.users.create_with_password("newUser", "newPassword", user_zone="tempZone") # 2. okay
groupadmin.users.create_with_password("newUser", "newPassword", user_zone="otherZone") # 3. okay
groupadmin.users.create_with_password("newUser", "newPassword", user_zone="fictionZone") # 4. okay - but error from server
groupadmin.users.create_with_password("newUser#tempZone", "newPassword") # 5. bad
groupadmin.users.create_with_password("newUser#otherZone", "newPassword") # 6. bad
groupadmin.users.create_with_password("newUser#fictionZone", "newPassword") # 7. bad
groupadmin.users.create_with_password("newUser#tempZone", "newPassword", user_zone="tempZone") # 8. bad
groupadmin.users.create_with_password("newUser#tempZone", "newPassword", user_zone="otherZone") # 9. bad
groupadmin.users.create_with_password("newUser#tempZone", "newPassword", user_zone="fictionZone") # 10. badThere was a problem hiding this comment.
I'm realizing that we kinda changed the requirements a little bit in this comment, so I'm going to update this to reflect that...
# tempZone is local; otherZone is remote; fictionZone does not exist
groupadmin.users.create_with_password("newUser", "newPassword") # 1. okay - use tempZone
groupadmin.users.create_with_password("newUser", "newPassword", user_zone="tempZone") # 2. okay
groupadmin.users.create_with_password("newUser", "newPassword", user_zone="otherZone") # 3. bad
groupadmin.users.create_with_password("newUser", "newPassword", user_zone="fictionZone") # 4. bad
groupadmin.users.create_with_password("newUser#tempZone", "newPassword") # 5. bad
groupadmin.users.create_with_password("newUser#otherZone", "newPassword") # 6. bad
groupadmin.users.create_with_password("newUser#fictionZone", "newPassword") # 7. bad
groupadmin.users.create_with_password("newUser#tempZone", "newPassword", user_zone="tempZone") # 8. bad
groupadmin.users.create_with_password("newUser#tempZone", "newPassword", user_zone="otherZone") # 9. bad
groupadmin.users.create_with_password("newUser#tempZone", "newPassword", user_zone="fictionZone") # 10. badThere was a problem hiding this comment.
Do we need to test all of the above? i.e. that they raise an exception where labelled as bad?
There was a problem hiding this comment.
It would be nice, but not strictly speaking required. Maybe we can just make an issue for it for later. I think it would be good to at least manually test these cases so that we know things are working as expected.
irods/manager/user_manager.py
Outdated
| _user_name = user_name + ("" if not user_zone else f"#{user_zone}") | ||
| else: | ||
| _user_name = user_name | ||
| user_name, user_zone = user_name.split("#") |
There was a problem hiding this comment.
it can be appended to help form the _user_name parameter in line 72.
Sorry, I should clarify. I was more referring to the fact that the user_zone parameter is not passed to the server directly. If provided, it is grafted to the user_name, but not sent as a separate parameter to the GeneralAdmin API.
Anyway, this conversation was originally about adding some pathological tests around this feature. Perhaps we should open a separate issue for that for later if things are working as intended now. Then, we can address that at another time and get this in for the next release.
No description provided.