[PATCH] Clarify debug message about initgroups and subdomains

[Sumit Bose]

Hi,

this patch fixes https://fedorahosted.org/sssd/ticket/1610 by changing
the debug message. See commit message for details.

bye,
Sumit

[Jakub Hrozek]

On Thu, Nov 08, 2012 at 11:11:20AM +0100, Sumit Bose wrote:
> Hi,
>
> this patch fixes https://fedorahosted.org/sssd/ticket/1610 by changing
> the debug message. See commit message for details.
>
> bye,
> Sumit

Ack

[Jakub Hrozek]

On Thu, Nov 08, 2012 at 12:31:17PM +0100, Jakub Hrozek wrote:
> On Thu, Nov 08, 2012 at 11:11:20AM +0100, Sumit Bose wrote:
> > Hi,
> >
> > this patch fixes https://fedorahosted.org/sssd/ticket/1610 by changing
> > the debug message. See commit message for details.
> >
> > bye,
> > Sumit
>
> Ack

Pushed to master and sssd-1-9

[Pavel Březina]

On 11/08/2012 12:48 PM, Jakub Hrozek wrote:
> On Thu, Nov 08, 2012 at 12:31:17PM +0100, Jakub Hrozek wrote:
> > On Thu, Nov 08, 2012 at 11:11:20AM +0100, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patch fixes https://fedorahosted.org/sssd/ticket/1610 by changing
> > > the debug message. See commit message for details.
> > >
> > > bye,
> > > Sumit
> >
> > Ack
>
> Pushed to master and sssd-1-9

Hi,
nack from me.

We should also change the code on ipa_id.c:80:

if (strcasecmp(ar->domain, breq->be_ctx->domain->name) != 0) {
         if (! ((ar->entry_type & BE_REQ_USER) ||
                (ar->entry_type & BE_REQ_GROUP))) {
             return sdap_handler_done(breq, DP_ERR_FATAL, EINVAL,
                                      "Invalid sub-domain request type");
         }

req = ipa_get_subdomain_account_info_send(...);
         if (!req) {
             return sdap_handler_done(breq, DP_ERR_FATAL, ENOMEM, ...);
         }

tevent_req_set_callback(req, ipa_account_info_users_done, breq);

return;
     }

This patch only works because we are wrongly using & instead of == in 
the condition above (BE_REQ* are not bitwise exclusive), therefore the 
condition passes for initgroups.

Is there any point for having this condition here? I think it would be 
better to remove it and let only ipa_get_subdomain_account_info_send() 
to decide what requests are valid.

[Sumit Bose]

On Thu, Nov 08, 2012 at 04:41:10PM +0100, Pavel Březina wrote:
> On 11/08/2012 12:48 PM, Jakub Hrozek wrote:
> > On Thu, Nov 08, 2012 at 12:31:17PM +0100, Jakub Hrozek wrote:
> > > On Thu, Nov 08, 2012 at 11:11:20AM +0100, Sumit Bose wrote:
> > > > Hi,
> > > >
> > > > this patch fixes https://fedorahosted.org/sssd/ticket/1610 by changing
> > > > the debug message. See commit message for details.
> > > >
> > > > bye,
> > > > Sumit
> > >
> > > Ack
> >
> > Pushed to master and sssd-1-9
>
> Hi,
> nack from me.
>
> We should also change the code on ipa_id.c:80:
> if (strcasecmp(ar->domain, breq->be_ctx->domain->name) != 0) {
>     if (! ((ar->entry_type & BE_REQ_USER) ||
>            (ar->entry_type & BE_REQ_GROUP))) {
>         return sdap_handler_done(breq, DP_ERR_FATAL, EINVAL,
>                                  "Invalid sub-domain request type");
>     }
>
>     req = ipa_get_subdomain_account_info_send(...);
>     if (!req) {
>         return sdap_handler_done(breq, DP_ERR_FATAL, ENOMEM, ...);
>     }
>
>     tevent_req_set_callback(req, ipa_account_info_users_done, breq);
>
>     return;
> }
>
> This patch only works because we are wrongly using & instead of ==
> in the condition above (BE_REQ* are not bitwise exclusive),
> therefore the condition passes for initgroups.

good catch
> Is there any point for having this condition here? I think it would
> be better to remove it and let only
> ipa_get_subdomain_account_info_send() to decide what requests are
> valid.

I agree, would you like to prepare the patch or shall I do?

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

On 11/08/2012 05:04 PM, Sumit Bose wrote:
> On Thu, Nov 08, 2012 at 04:41:10PM +0100, Pavel Březina wrote:
> > On 11/08/2012 12:48 PM, Jakub Hrozek wrote:
> > > On Thu, Nov 08, 2012 at 12:31:17PM +0100, Jakub Hrozek wrote:
> > > > On Thu, Nov 08, 2012 at 11:11:20AM +0100, Sumit Bose wrote:
> > > > > Hi,
> > > > >
> > > > > this patch fixes https://fedorahosted.org/sssd/ticket/1610 by changing
> > > > > the debug message. See commit message for details.
> > > > >
> > > > > bye,
> > > > > Sumit
> > > >
> > > > Ack
> > >
> > > Pushed to master and sssd-1-9
> >
> > Hi,
> > nack from me.
> >
> > We should also change the code on ipa_id.c:80:
> >  if (strcasecmp(ar->domain, breq->be_ctx->domain->name) != 0) {
> >      if (! ((ar->entry_type & BE_REQ_USER) ||
> >             (ar->entry_type & BE_REQ_GROUP))) {
> >          return sdap_handler_done(breq, DP_ERR_FATAL, EINVAL,
> >                                   "Invalid sub-domain request type");
> >      }
> >
> >      req = ipa_get_subdomain_account_info_send(...);
> >      if (!req) {
> >          return sdap_handler_done(breq, DP_ERR_FATAL, ENOMEM, ...);
> >      }
> >
> >      tevent_req_set_callback(req, ipa_account_info_users_done, breq);
> >
> >      return;
> >  }
> >
> > This patch only works because we are wrongly using & instead of ==
> > in the condition above (BE_REQ* are not bitwise exclusive),
> > therefore the condition passes for initgroups.
>
> good catch
> > Is there any point for having this condition here? I think it would
> > be better to remove it and let only
> > ipa_get_subdomain_account_info_send() to decide what requests are
> > valid.
>
> I agree, would you like to prepare the patch or shall I do?

Well, I didn't really plan to do it but there is no point to be delayed 
but communication (although I usually screw these simple patches) :) 
Patch is attached.
> bye,
> Sumit

[Jakub Hrozek]

On Thu, Nov 08, 2012 at 05:56:25PM +0100, Pavel Březina wrote:
> On 11/08/2012 05:04 PM, Sumit Bose wrote:
> > On Thu, Nov 08, 2012 at 04:41:10PM +0100, Pavel Březina wrote:
> > > On 11/08/2012 12:48 PM, Jakub Hrozek wrote:
> > > > On Thu, Nov 08, 2012 at 12:31:17PM +0100, Jakub Hrozek wrote:
> > > > > On Thu, Nov 08, 2012 at 11:11:20AM +0100, Sumit Bose wrote:
> > > > > > Hi,
> > > > > >
> > > > > > this patch fixes https://fedorahosted.org/sssd/ticket/1610 by changing
> > > > > > the debug message. See commit message for details.
> > > > > >
> > > > > > bye,
> > > > > > Sumit
> > > > >
> > > > > Ack
> > > >
> > > > Pushed to master and sssd-1-9
> > >
> > > Hi,
> > > nack from me.
> > >
> > > We should also change the code on ipa_id.c:80:
> > > if (strcasecmp(ar->domain, breq->be_ctx->domain->name) != 0) {
> > >     if (! ((ar->entry_type & BE_REQ_USER) ||
> > >            (ar->entry_type & BE_REQ_GROUP))) {
> > >         return sdap_handler_done(breq, DP_ERR_FATAL, EINVAL,
> > >                                  "Invalid sub-domain request type");
> > >     }
> > >
> > >     req = ipa_get_subdomain_account_info_send(...);
> > >     if (!req) {
> > >         return sdap_handler_done(breq, DP_ERR_FATAL, ENOMEM, ...);
> > >     }
> > >
> > >     tevent_req_set_callback(req, ipa_account_info_users_done, breq);
> > >
> > >     return;
> > > }
> > >
> > > This patch only works because we are wrongly using & instead of ==
> > > in the condition above (BE_REQ* are not bitwise exclusive),
> > > therefore the condition passes for initgroups.
> >
> > good catch
> > > Is there any point for having this condition here? I think it would
> > > be better to remove it and let only
> > > ipa_get_subdomain_account_info_send() to decide what requests are
> > > valid.
> >
> > I agree, would you like to prepare the patch or shall I do?
>
> Well, I didn't really plan to do it but there is no point to be
> delayed but communication (although I usually screw these simple
> patches) :) Patch is attached.

Ack

[Jakub Hrozek]

On Mon, Nov 12, 2012 at 03:06:14PM +0100, Jakub Hrozek wrote:
> On Thu, Nov 08, 2012 at 05:56:25PM +0100, Pavel Březina wrote:
> > On 11/08/2012 05:04 PM, Sumit Bose wrote:
> > > On Thu, Nov 08, 2012 at 04:41:10PM +0100, Pavel Březina wrote:
> > > > On 11/08/2012 12:48 PM, Jakub Hrozek wrote:
> > > > > On Thu, Nov 08, 2012 at 12:31:17PM +0100, Jakub Hrozek wrote:
> > > > > > On Thu, Nov 08, 2012 at 11:11:20AM +0100, Sumit Bose wrote:
> > > > > > >Hi,
> > > > > > >
> > > > > > >this patch fixes https://fedorahosted.org/sssd/ticket/1610 by changing
> > > > > > >the debug message. See commit message for details.
> > > > > > >
> > > > > > >bye,
> > > > > > >Sumit
> > > > > >
> > > > > > Ack
> > > > >
> > > > > Pushed to master and sssd-1-9
> > > >
> > > > Hi,
> > > > nack from me.
> > > >
> > > > We should also change the code on ipa_id.c:80:
> > > > if (strcasecmp(ar->domain, breq->be_ctx->domain->name) != 0) {
> > > >     if (! ((ar->entry_type & BE_REQ_USER) ||
> > > >            (ar->entry_type & BE_REQ_GROUP))) {
> > > >         return sdap_handler_done(breq, DP_ERR_FATAL, EINVAL,
> > > >                                  "Invalid sub-domain request type");
> > > >     }
> > > >
> > > >     req = ipa_get_subdomain_account_info_send(...);
> > > >     if (!req) {
> > > >         return sdap_handler_done(breq, DP_ERR_FATAL, ENOMEM, ...);
> > > >     }
> > > >
> > > >     tevent_req_set_callback(req, ipa_account_info_users_done, breq);
> > > >
> > > >     return;
> > > > }
> > > >
> > > > This patch only works because we are wrongly using & instead of ==
> > > > in the condition above (BE_REQ* are not bitwise exclusive),
> > > > therefore the condition passes for initgroups.
> > >
> > > good catch
> > > > Is there any point for having this condition here? I think it would
> > > > be better to remove it and let only
> > > > ipa_get_subdomain_account_info_send() to decide what requests are
> > > > valid.
> > >
> > > I agree, would you like to prepare the patch or shall I do?
> >
> > Well, I didn't really plan to do it but there is no point to be
> > delayed but communication (although I usually screw these simple
> > patches) :) Patch is attached.
>
> Ack

Pushed to master and sssd-1-9