[SSSD] [sssd PR#311][comment] RESPONDER: Use fqnames as output when needed

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/311
Title: #311: RESPONDER: Use fqnames as output when needed

jhrozek commented:
"""
On Tue, Jun 20, 2017 at 05:33:08AM -0700, fidencio wrote:
>  On Tue, Jun 20, 2017 at 11:33 AM, Jakub Hrozek
> <notifications(a)github.com&gt;
>  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. 
Yeah, I don't like the side-effect of calling
`cache_req_domain_new_list_from_domain_resolution_order` and having the
domain property magically changed.

Could you please add a comment to
`cache_req_domain_new_list_from_domain_resolution_order` that this
happens? I also wonder if we should mark the domain parameter as
'output' with an underscore, but that's usually done for parameters that
the function constructs.

>  
>  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!
>  
>  
>  > —
>  > You are receiving this because you authored the thread.
>  > Reply to this email directly, view it on GitHub
>  > <https://github.com/SSSD/sssd/pull/311#pullrequestreview-45080418>;, or mute
>  > the thread
>  >
> <https://github.com/notifications/unsubscribe-auth/AAG4euYkOQZoeo6dH4b2frs...
>  > .
>  >
>  
>  
>  -- 
>  You are receiving this because you were assigned.
>  Reply to this email directly or view it on GitHub:
>  https://github.com/SSSD/sssd/pull/311#issuecomment-309738765 
"""

See the full comment at https://github.com/SSSD/sssd/pull/311#issuecomment-309991986