Update testing to allow for nonadmin and start RonR removal #190
Conversation
Signed-off-by: Chris Mitchell <[email protected]>
|
Finally, FINALLY this is in a place where it could hit the repo. There's likely something I'm missing in here, let me know if y'all find anything, please! |
rbac/rbac/middleware.py
Outdated
| """Obtain access data for given username.""" | ||
| principal = None | ||
| access_list = None | ||
| def _get_access_for_user(username, tenant): # pylint: disable=too-many-locals,too-many-branches,unused-argument |
There was a problem hiding this comment.
Instead of adding the exception for unused-argument here, can we just pull out username, tenant here and in the caller? You'd probably have to update the test callers as well.
There was a problem hiding this comment.
We could, but my worry there was that, since we're attempting to keep the function as is to allow minimal changes to other code, changing its arguments and signature would then require changing any call to the function. I could look into where it's called and fix each call up if there's only a couple, but wanted to go for minimal change where possible.
There was a problem hiding this comment.
Gotcha, makes sense. I did a global search on the repo and it looks like it's just the one spot calling it (along with tests).
| @@ -109,7 +110,7 @@ def _create_request_context(cls, customer_data, user_data, | |||
| 'user': { | |||
| 'username': user_data['username'], | |||
| 'email': user_data['email'], | |||
| 'is_org_admin': True | |||
| 'is_org_admin': is_org_admin | |||
tests/management/access/test_view.py
Outdated
|
|
||
| def test_admin_RonR(self): | ||
| """Test that a admin user can access RBAC resources""" | ||
| url = '{}?application={}&username={}'.format(reverse('access'), 'rbac', self.principal.username) |
There was a problem hiding this comment.
So, this is checking against the access endpoint, correct? Like /access/?application=rbac? Shouldn't that always return a 200, just with an empty dataset, whereas actual RBAC resources (groups/roles, etc) would be limited by this change? I may be missing something, though. 🤔
There was a problem hiding this comment.
I went back and forth on which endpoint to test, ended up hitting access primarily to have a decision made and something to review. I honestly wasn't sure how access was to work after this change with a non-admin user, but that may not be the case based on your testing below. I think for ease of use and understanding I'll swap these tests to a different endpoint and then maybe step through execution to determine what's happening on the non-adming access test
tests/management/access/test_view.py
Outdated
|
|
||
| def test_nonadmin_RonR(self): | ||
| """Test that a nonadmin user can't access RBAC resources""" | ||
| url = '{}?application={}&username={}'.format(reverse('access'), 'rbac', self.principal.username) |
There was a problem hiding this comment.
This is the one I'd expect to still be a 200 as a non-admin, so I must be mistaken in how I'm interpreting these tests. If I test the access endpoint locally as a non-admin, I get a 200 with an empty set for RBAC as expected, but this appears to 403? If I check the principals, groups, or roles endpoints I do get a 403 which I would also expect.
There was a problem hiding this comment.
@coderbydesign Do you mean when a non-admin want to get access for himself, it is reasonable to return 200 with empty set.
But when he wants to get access for others, it should be returning forbidden?
There was a problem hiding this comment.
Yeah. Is that what's happening? The access query isn't for the principal on the request? That would make sense.
Signed-off-by: Chris Mitchell <[email protected]>
Signed-off-by: Chris Mitchell [email protected]