[SSSD] [PATCHES] SDAP: refactor sdap_access

Pavel Reichl preichl at redhat.com
Thu Aug 14 12:16:21 UTC 2014


On 08/14/2014 12:09 PM, Pavel Březina wrote:
> On 08/13/2014 05:06 PM, Pavel Reichl wrote:
>>
>> On 08/13/2014 02:32 PM, Pavel Březina wrote:
>>> On 08/13/2014 10:49 AM, Pavel Reichl wrote:
>>>>
>>>> On 08/12/2014 02:16 PM, Pavel Březina wrote:
>>>>> On 08/11/2014 05:55 PM, Pavel Reichl wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I was struggling with rebasing my patches in the thread "SDAP: Be 
>>>>>> able
>>>>>> to configure sssd to honor openldap account lock to restrict access
>>>>>> via
>>>>>> ssh key" so I decided to split the patch set into smaller chunks 
>>>>>> to be
>>>>>> able to quickly react on review comments and not to fight with
>>>>>> rebasing
>>>>>> all the time.
>>>>>>
>>>>>> Attached patches prepare code for implementing #2364
>>>>>>
>>>>>> Patch:
>>>>>> 1st) acked by Pavel
>>>>>> 2nd) Jakub requsted change:
>>>>>>> +static errno_t sdap_get_basedn_user_entry(TALLOC_CTX *mem_ctx,
>>>>>>> +                                          struct ldb_message
>>>>>>> *user_entry,
>>>>>>> +                                          const char *username,
>>>>>>> +                                          char **_basedn)
>>>>>>> +{
>>>>>>> +    const char *basedn;
>>>>>>> +    errno_t ret;
>>>>>>> +
>>>>>>> +    basedn = ldb_msg_find_attr_as_string(user_entry, 
>>>>>>> SYSDB_ORIG_DN,
>>>>>>> NULL);
>>>>>>> +    if (basedn == NULL) {
>>>>>>> +        DEBUG(SSSDBG_CRIT_FAILURE,"Could not find originalDN for
>>>>>>> user
>>>>>>> [%s]\n",
>>>>>>> +              username);
>>>>>>> +        ret = EINVAL;
>>>>>>> +        goto done;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    *_basedn = talloc_strdup(mem_ctx, basedn);
>>>>>>> +    if (*_basedn == NULL) {
>>>>>>> +        DEBUG(SSSDBG_CRIT_FAILURE,
>>>>>>> +              "Could not allocate memory for originalDN\n");
>>>>>>> +        ret = ENOMEM;
>>>>>>> +        goto done;
>>>>>>> +    }
>>>>>>> Do we need to strdup the basedn here?
>>>>>>>
>>>>>> 3rd)  acked by Pavel
>>>>>> 4th) Jakub comment:
>>>>>>
>>>>>>> This function is misplaced, it immediatelly follows _send() in the
>>>>>>> code.
>>>>>>> _recv() should be the last in the flow.
>>>>>>
>>>>>> 5th) acked by Pavel
>>>>>>
>>>>>> I hope attached patches address Jakub's concerns.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Pavel Reichl
>>>>>
>>>>> Patch 1: Ack.
>>>>> Patch 2: Ack. Just the commit message needs some line wrapping.
>>>>> Patch 3: Ack.
>>>>> Patch 4:
>>>>> It is good to order function by its execution time when you work in
>>>>> tevent. I.e. the correct order should be:
>>>>> _send
>>>>> check_next_rule (which would be nice to also rename to
>>>>> sdap_access_next_rule or similar)
>>>>> _filter_done
>>>>> _done
>>>>> _recv
>>>>
>>>> OK, I renamed the function and updated the function definition 
>>>> order as
>>>> you proposed.
>>>>
>>>> But I'm wondering whether according to the rule you described
>>>> sdap_access_filter_send should  immediately follow check_next_rule 
>>>> as it
>>>> is called before sdap_access_filter_done?
>>>
>>> Hi,
>>> this is unfortunately cause of poor naming. Thanks for pointing it
>>> out, we will fix that now :-)
>>>
>>> What do we have now are two tevent requests:
>>> 1) sdap_access_send
>>>    - sdap_access_check_next_rule
>>>    - sdap_access_filter_done
>>>    - sdap_access_lock_done (future patch)
>>>    - sdap_access_done
>>>    - sdap_access_subreq_recv
>>>    - sdap_access_recv
>>>
>>> sdap_access_check_next_rule can either:
>>> a) create a new subrequest for filter check, setting
>>> sdap_access_filter_done as callback
>>> b) same as a but for lock in future patches
>>> c) finish the request 1
>>>
>>> 2) sdap_access_filter_send
>>>    - sdap_access_filter_retry
>>>    - sdap_access_filter_connect_done
>>>    - sdap_access_filter_get_access_done
>>>    - sdap_access_filter_recv
>>>
>>> It is hard to follow such code. We should improve it this way:
>> Hmm I personally don't think this is much an improvement. Anyway, I
>> (again) updated patches as you instructed, I hope I did it as you wished
>> and that you can finally (again) ACK the patches.
>
> You just forgot to do one change - move sdap_access_filter_recv after 
> sdap_access_filter_done.
>
> And I just notice one more thing - you access state of 
> sdap_access_filter outside of the request - that is bad.
>
> - move sdap_access_filter_req_ctx before sdap_access_filter_send. 
> State structures should be named $prefix_state, this is an old code so 
> I don't require you to rename it now, since the whole file uses this 
> $prefix_req_ctx notation, but remember that when you create new tevent 
> request.
>
> - ac_type field you created should be part of sdap_access_req_ctx
>
OK, thanks for noticing and explanation!
>>>
>>> 1) sdap_access_send
>>>    - sdap_access_check_next_rule
>>>    - sdap_access_done
>>>    - sdap_access_recv
>>>
>>> We will remove sdap_access_{filter|lock}_done, they only call
>>> sdap_access_done anyway. We will move code from
>>> sdap_access_subreq_recv to sdap_access_done directly. There is no code
>>> duplication anyway so you don't have to be worried about it.
>>>
>>> 2) sdap_access_filter_send
>>>    - sdap_access_filter_retry
>>>    - sdap_access_filter_connect_done
>>>    - sdap_access_filter_done
>>>    - sdap_access_filter_recv
>>>
>>> We will rename sdap_access_filter_get_access_done to
>>> sdap_access_filter_done.
>>>
>>> Tevent guideline is to start each request with $prefix_send, continue
>>> with $prefix_$part_step or $prefix_$part_done, end it with
>>> $prefix_done and than allow the caller to fetch result with
>>> $prefix_recv. The code should be read from top to bottom without
>>> interruption.
>>>
>>>>> Patch 5: Ack.
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SDAP-split-sdap_access_filter_get_access_done.patch
Type: text/x-patch
Size: 3813 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140814/8d1b136f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-SDAP-refactor-sdap_access_filter_send.patch
Type: text/x-patch
Size: 3563 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140814/8d1b136f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-SDAP-nitpicks-in-sdap_access_filter_get_access_done.patch
Type: text/x-patch
Size: 2137 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140814/8d1b136f/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-SDAP-refactor-sdap_access_filter_done.patch
Type: text/x-patch
Size: 7210 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140814/8d1b136f/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-SDAP-don-t-log-error-on-access-denied.patch
Type: text/x-patch
Size: 1149 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140814/8d1b136f/attachment-0004.bin>


More information about the sssd-devel mailing list