[SSSD] [PATCHES] Make nss responder aware of views and overrides

Pavel Březina pbrezina at redhat.com
Fri Oct 17 12:05:44 UTC 2014


On 10/15/2014 01:08 PM, Sumit Bose wrote:
> On Tue, Oct 14, 2014 at 05:57:36PM +0200, Sumit Bose wrote:
>> Hi,
>>
>> this patches make the nss responder view aware. A number of new sysdb
>> calls is added as drop in replacements from existing ones. I preferred
>> this instead of modifying the existing ones to not break their usage in
>> other places. But I think they are a good targets, together with the
>> other user and group related sysdb calls, for a refactoring in an
>> upcoming version.
>>
>> bye,
>> Sumit
>
> Please find attached a new version with some minor fixes in fill_pwent()
> and fill_grent() with respect to fully qualified names and an additional
> patch with the "changes" for the pam responder.
>
> bye,
> Sumit

Patch #4 sysdb: add overide lookup calls won't apply to current master. 
But the rebase was trivial so it didn't stop me from reviewing.

Patch #1: confdb: add has_views and view_name to sss_domain_info

ACK

Patch #2: new_subdomain: copy view data from parent

ACK

Patch #3: sysdb: add view data to domains

> +    if (ret == ENOENT || view_name == NULL
> +            || (view_name != NULL
> +                && strcmp(view_name, SYSDB_DEFAULT_VIEW_NAME) == 0)) {

view_name != NULL condition can be skipped

And the whole branching in sysdb_master_domain_update() is quite complex 
and difficult to understand. Can you add a comment to the top of each 
branch that says when the branch will be executed?

Patch #4: sysdb: add overide lookup calls

ACK

Patch #5: sysdb: add sysdb_getpwnam/uid_with_views()

> +    /* TODO: check if EOK is returned even if orig_obj is empty, i.e nothgin
> +     * found. */

This is strange todo to leave in the code. Can you do it? :-)

Patch #6: sysdb: add sss_view_ldb_msg_find_element/attr_as_string/uint64

ACK

Patch #7:  nss: add view support for getpwnam/getpwuid requests

I think you can use sss_view_ldb* functions from the previous patch, 
can't you?

Patch #8: sysdb: add sysdb_initgroups_with_views()

ACK

Patch #9: nss: add view support to initgroups request

ACK

Patch #10: sysdb: add sysdb_getgrnam_with_views and 
sysdb_getgrgid_with_views

> +    /* TODO: check if EOK is returned even if orig_obj is empty, i.e nothing
> +     * found. */

Again.

Patch #11: nss: add view support for getgr* requests

Coverity warning:

nsssrv_cmd.c:2856: value_overwrite: Value "22" is overwritten with value 
from "sss_packet_set_size(packet, 0UL)".
nsssrv_cmd.c:2810: assigned_value: Value "22" is assigned to "ret" here, 
but that stored value is not used before it is overwritten.

Patch #12: sid2name: return name without views applied

> +    if (apply_no_view) {
> +        orig_name = ldb_msg_find_attr_as_string(msg,
> +                                                ORIGINALAD_PREFIX SYSDB_NAME,
> +                                                NULL);
> +    }
> +
> +    if (orig_name == NULL) {
> +        orig_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
> +    }

The name of the variable 'apply_no_view' suggests that if true, no 
override should be applied. Thus if false, shouldn't we use 
sss_view_ldb* to get overriden value if possible?

Patch #13: pam: make pam responder aware if views

ACK

I was using these patches for quite some time during my work on IFP and 
sudo responder part of the views. All known issues seems to be resolved. 
Functional ACK.




More information about the sssd-devel mailing list