URL:
https://github.com/SSSD/sssd/pull/311
Title: #311: RESPONDER: Use fqnames as output when needed
fidencio commented:
"""
On Tue, Jun 20, 2017 at 11:33 AM, Jakub Hrozek <notifications(a)github.com>
wrote:
*@jhrozek* requested changes on this pull request.
We should think about not modifying the sss_domain_info structure..
------------------------------
In src/responder/common/cache_req/cache_req_domain.c
<
https://github.com/SSSD/sssd/pull/311#discussion_r122925383>:
> @@ -155,6 +161,14 @@ cache_req_domain_new_list_from_string_list(TALLOC_CTX
*mem_ctx,
cr_domain->fqnames =
cache_req_domain_use_fqnames(dom, enforce_non_fqnames);
+ /* when using the domain resolution order, using shortnames as input
+ * is allowed by default. However, we really want to use the fully
+ * qualified name as output in order to avoid conflicts whith users
+ * who have the very same name. */
+ if (resolution_order != NULL) {
+ cr_domain->domain->output_fqnames = true;
I'm sorry I didn't notice this the first time I was reviewing the patch,
but I don't like the cache_req request code modifying the domain property.
Could we please do this instead at the time the resolution order is read?
Jakub, I'm not sure if I understand your suggestion here.
Although I really dislike playing with domain's property from the cache_req
code, I do believe a setter would be enough for making it cleaner.
Setting those attributes in the place where we flatten the list (as it's
done with this patch) seems the saner approach to be taken (+ the setter
than has been introduced in the last pushed series).
Please, let me know what was your idea so I could, at least, try to follow
your suggestion in case you don't "buy" my approach.
Additionally, I wonder if we could have a setter for the
output_fqnames
property. We've been wanting to make the sss_domain_info opaque precisely
to control who can set what.
Done!
"""
See the full comment at
https://github.com/SSSD/sssd/pull/311#issuecomment-309738765