cache_req: use sysdb_search_user_by_upn_with_view_res() #7998
Conversation
|
Is this related to #7996? |
|
|
Do you know if it's applicable to sssd-2-9-4? |
Yes, looks like the issue is present since a long time. |
pbrezina
force-pushed
the
master
branch
2 times, most recently
from
f5d64b3 to
b854636
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors user lookup by UPN to use a new function sysdb_search_user_by_upn_with_view_res that correctly applies overrides from views. The changes involve removing the old sysdb_getpwupn function, implementing the new one, and updating the cache request plugin and tests. The overall change is good and includes a relevant system test. I found one potential logic bug in the new function where an ENOENT error code might be incorrectly overwritten, which I've commented on with a suggested fix.
| /* If there are views we have to check if override values must be added to | ||
| * the original object. */ |
There was a problem hiding this comment.
There is a potential logic error in this function. If sysdb_search_user_by_upn_res returns ENOENT, ret is set to ENOENT. The code then proceeds to line 759, where ret is unconditionally overwritten to EOK. This causes the function to return EOK with an empty result set, instead of ENOENT, which was the original error code indicating the entry was not found. This changes the semantics of the function's return value.
To preserve the ENOENT error code, you should handle this case before ret is overwritten. I suggest adding a check after the initial search and before attempting to process overrides.
if (ret == ENOENT) {
goto done;
}
/* If there are views we have to check if override values must be added to
* the original object. */
There was a problem hiding this comment.
This is valid. The original code returns the empty object but keeps ENOENT return code.
There was a problem hiding this comment.
I mean, what's the point, why not simply if (ret != EOK) return ret;?
There was a problem hiding this comment.
Hi,
I fixed this as Pavel suggested, the function will return ENOENT with an empty (not NULL) struct ldb_result as sysdb_search_user_by_upn_res(). The caller in the cache_req code is quite robust and could handle the original version as well, but being consistent with sysdb_search_user_by_upn_res() makes sense imo.
bye,
Sumit
There was a problem hiding this comment.
Ok, perhaps empty object is expected by the caller of 'cache_req_plugin::lookup_fn' (I didn't check).
But current patch returns NULL in case sysdb_search_user_by_upn_res() == ENOENT, not empty struct...
There was a problem hiding this comment.
Ok, perhaps empty object is expected by the caller of 'cache_req_plugin::lookup_fn' (I didn't check).
No, it isn't expected if ENOENT is returned.
But current patch returns NULL in case
sysdb_search_user_by_upn_res() == ENOENT, not empty struct...
I think unfortunately both cases are possible. But in either case sysdb_search_user_by_upn_with_view_res() would return the same combination as sysdb_search_user_by_upn_res().
There was a problem hiding this comment.
Ok, perhaps empty object is expected by the caller of 'cache_req_plugin::lookup_fn' (I didn't check).
No, it isn't expected if
ENOENTis returned.
I'm confused: what is the point then to orig_obj = talloc_zero(mem_ctx, struct ldb_result); if sysdb_add_overrides_to_object() == ENOENT?
There was a problem hiding this comment.
Hi,
in the latest version I simplified sysdb_search_user_by_upn_with_view_res() wit hthe result that it might behaver differently than other lookup_fn methods of cache_req plugins but not cause issues in the single caller.
bye,
Sumit
There was a problem hiding this comment.
Thank you.
@pbrezina, are you fine with the latest version?
| /* If there are views we have to check if override values must be added to | ||
| * the original object. */ |
There was a problem hiding this comment.
This is valid. The original code returns the empty object but keeps ENOENT return code.
|
Note: Covscan is green. |
|
f-44 system test fails aren't due to this PR. |
The new call will apply overrides to a user object which was searched by UPN or email address before returning it. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Pavel Březina <pbrezina@redhat.com>
To make sure any overrides are applied to the user even when searched by UPN or email address sysdb_search_user_by_upn_with_view_res() is now used in the cache request code. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Pavel Březina <pbrezina@redhat.com>
Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Pavel Březina <pbrezina@redhat.com>
Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Pavel Březina <pbrezina@redhat.com>
Add a system test to verify that IPA ID view overrides are correctly applied when looking up a user by email address. The test creates a user with an email, applies ID view overrides (login, uid, gid, home), and verifies that the overridden values are returned when looking up the user by: - original name - overridden name - email address Signed-off-by: Madhuri Upadhye <mupadhye@redhat.com> Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Pavel Březina <pbrezina@redhat.com>
5 participants
To make sure any overrides are applied to the user even when searched by
UPN or email address sysdb_search_user_by_upn_with_view_res() is now used
in the cache request code.