-
Notifications
You must be signed in to change notification settings - Fork 270
cache_req: use sysdb_search_user_by_upn_with_view_res() #7998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+158
−49
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c3612fc
sysdb: add sysdb_search_user_by_upn_with_view_res()
sumit-bose d269e98
cache_req: use sysdb_search_user_by_upn_with_view_res()
sumit-bose b839aa0
sysdb:: remove sysdb_getpwupn()
sumit-bose deeba8a
tests: lookup user with overrides with email
sumit-bose 6ad1983
tests: add IPA ID view test for user lookup by email
madhuriupadhye File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a potential logic error in this function. If
sysdb_search_user_by_upn_resreturnsENOENT,retis set toENOENT. The code then proceeds to line 759, whereretis unconditionally overwritten toEOK. This causes the function to returnEOKwith an empty result set, instead ofENOENT, 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
ENOENTerror code, you should handle this case beforeretis overwritten. I suggest adding a check after the initial search and before attempting to process overrides.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is valid. The original code returns the empty object but keeps ENOENT return code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'empty object' == NULL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, what's the point, why not simply
if (ret != EOK) return ret;?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I fixed this as Pavel suggested, the function will return
ENOENTwith an empty (notNULL)struct ldb_resultassysdb_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 withsysdb_search_user_by_upn_res()makes sense imo.bye,
Sumit
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it isn't expected if
ENOENTis returned.I think unfortunately both cases are possible. But in either case
sysdb_search_user_by_upn_with_view_res()would return the same combination assysdb_search_user_by_upn_res().There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused: what is the point then to
orig_obj = talloc_zero(mem_ctx, struct ldb_result);ifsysdb_add_overrides_to_object() == ENOENT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
in the latest version I simplified
sysdb_search_user_by_upn_with_view_res()wit hthe result that it might behaver differently than otherlookup_fnmethods of cache_req plugins but not cause issues in the single caller.bye,
Sumit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
@pbrezina, are you fine with the latest version?