federationuser(unittest): update to federate/link the checked entity before we do an entity search (PROJQUAY-5137) #2635
Conversation
|
Can one of the admins verify this patch? |
michaelalang
changed the title
…cool_user I assume its fine to update the check accordingly
…but the test is mocking ldap so it should be user
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2635 +/- ##
==========================================
+ Coverage 70.69% 70.71% +0.01%
==========================================
Files 435 436 +1
Lines 40105 40211 +106
Branches 5215 5227 +12
==========================================
+ Hits 28352 28434 +82
- Misses 10099 10122 +23
- Partials 1654 1655 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| # we need to ensure, the user is federate before we can check it | ||
| with mock_ldap() as ldap: | ||
| # Verify that the user is logged in and their username was adjusted. | ||
| (response, _) = ldap.verify_and_link_user("cool.user", "somepass") |
There was a problem hiding this comment.
I think this only applies when auth_engine is mock_ldap.
Can only apply this when auth_engine is set tomock_ldap. Same thing for the assumption changes below.
There was a problem hiding this comment.
@kleesc I do not understand what you mean in that regards. If I got it right you mean the change should check if the auth_engine is mock_ldap ?
I doubt as even for the other fixtures listed the federation has to be finished. I have not seen a possibility to authenticate with the full federation being finished/setup.
There was a problem hiding this comment.
Right, since the mock_ldap context wouldn't be relevant to fake_jwt or fake_keystone?
There was a problem hiding this comment.
check ... done thanks @kleesc
… and none federated users
the Unittest for entity searching requires to be updated to first federate (link) the searched user into the Database.
Something like seen below.
In other words, the Unittest has been assuming wrongly up until now that the prefix=cool users was already federated when hitting the EntitySearch.
So its an ordering issue we need to address.
Compared to real-life implementations, the User method verify_and_link_user has not been called when executing, which happens at normally at login.
All other unit-tests called prior test_entity_search are not executing the method which explains why it's failing with the change to not do LDAP lookup for Robot accounts that are in the Database only.
Manual verification in a LAB show that the PR change does work as expected as the method verify_and_link_user in users/federation calls module externalldap.py and the method verify_credentials which does the LDAP lookup accordingly.
Additional reason why the context for LDAP lookup in the verify_credentials method is sufficient, from Issue PROJQUAY-5117 where it's stated, that the call is necessary because of permissions granting to the Robot, I want to challenge that for this being true, it would be necessary that Robot accounts are configured in LDAP too which is not the case.
Furthermore, in relation to PROJQUAY-5657 I was able to verify that as long s the session has not expired, Users can be removed from LDAP completely and still get access granted, obsoleting the statement for necessity once more,.
Code review description
concept
The update to the unittest relates to PROJQUAY-5968 as well as PROJQUAY-5117.
Both of them dealing with disabling robot accounts completely and disabling LDAP lookups on robot accounts as those are only kept in the database.
the update on the Unittest for
endpoints/api/test/test_endtoend_auth.pychanges the behavior where up to this fix, the test assumed that thecool.userwas federated when doing an entity search which is not correct in particular when having none Database setups. Furthermore, even with Database setups it should return atype: userinstead oftype: external.expected behavior classification
the Unittest fix changes the behavior to be closer to the real implementation:
kind: userkind: userfaulty behavior classification
the Unittest might not cover unknown (to me) entities which are returned as
kind: external. I wasn't able to identify how Quay will return such entities in real-deployment scenarios