5:33 a.m.
Hi,
with these four patches the SID-to-name API can now also be used with
the AD provider and for local IPA accounts. Since this goes beyond the
functionality needed by the FreeIPA WebUI I send them in a separate
series. The patches are also a requirement for using the PAC with the AD
provider (https://fedorahosted.org/sssd/ticket/1558).
bye,
Sumit
11:07 a.m.
On Mon, May 13, 2013 at 12:33:46PM +0200, Sumit Bose wrote:
Hi,
with these four patches the SID-to-name API can now also be used with
the AD provider and for local IPA accounts. Since this goes beyond the
functionality needed by the FreeIPA WebUI I send them in a separate
series. The patches are also a requirement for using the PAC with the AD
provider (https://fedorahosted.org/sssd/ticket/1558).
bye,
Sumit
[PATCH 1/4] IPA: Always initialize ID mapping
sdap_auth_ctx->service = ipa_options->service->sdap;
ipa_options->auth_ctx->sdap_auth_ctx = sdap_auth_ctx;
- ret = ipa_get_id_options(ipa_options, bectx->cdb, bectx->conf_path,
- &sdap_auth_ctx->opts);
- if (ret != EOK) {
+ if (ipa_options->id == NULL) {
+ ret = EINVAL;
goto done;
}
+ sdap_auth_ctx->opts = ipa_options->id;
Ack, I would just like to move all assignments to sdap_auth_ctx together
and then assign to ipa_options->auth_ctx for better readability.
[PATCH 2/4] Handle SID strings in sdap_attrs_get_sid_str() as well
+ if (el->values[0].length > 2 &&
+ el->values[0].data[0] == 'S' &&
+ el->values[0].data[1] == '-')
+ {
Ack I will just reformat this opening bracket to conform to our coding
guidelines.
+ sid_str = talloc_strndup(mem_ctx, (char *)
el->values[0].data,
+ el->values[0].length);
+ if (sid_str == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, ("talloc_strndup failed.\n"));
+ return ENOMEM;
+ }
+ } else {
+ err = sss_idmap_bin_sid_to_sid(idmap_ctx->map,
+ el->values[0].data,
+ el->values[0].length,
+ &sid_str);
+ if (err != IDMAP_SUCCESS) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Could not convert SID: [%s]\n",
+ idmap_error_string(err)));
+ return EIO;
+ }
}
*_sid_str = talloc_steal(mem_ctx, sid_str);
--
1.7.7.6
[PATCH 3/4] IPA: read user and group SID
Ack.
From 8f59f19435d9d0e349c957f6abf698377f21a80f Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Tue, 7 May 2013 14:39:42 +0200
Subject: [PATCH 4/4] Add SID related requests to the LDAP provider
The patch adds support for BE_REQ_BY_SECID and BE_REQ_USER_AND_GROUP to
the LDAP provider. Since the AD and the IPA provider use the same code
they support those request now as well.
Besides allowing that users and groups can be searched by the SID as
well the new request allows to search users and groups in one run, i.e.
if there is not user matching the search criteria groups are searched as
well.
---
src/providers/ldap/ldap_id.c | 216 +++++++++++++++++++++++++++++++++++++++++-
1 files changed, 213 insertions(+), 3 deletions(-)
diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
index 4d373a4..c4e8dbf 100644
--- a/src/providers/ldap/ldap_id.c
+++ b/src/providers/ldap/ldap_id.c
@@ -136,6 +136,14 @@ struct tevent_req *users_get_send(TALLOC_CTX *memctx,
}
}
break;
+ case BE_FILTER_SECID:
+ attr_name = ctx->opts->user_map[SDAP_AT_USER_OBJECTSID].name;
+
+ ret = sss_filter_sanitize(state, name, &clean_name);
+ if (ret != EOK) {
+ goto fail;
+ }
+ break;
default:
ret = EINVAL;
goto fail;
@@ -446,14 +454,22 @@ struct tevent_req *groups_get_send(TALLOC_CTX *memctx,
}
}
break;
+ case BE_FILTER_SECID:
+ attr_name = ctx->opts->user_map[SDAP_AT_USER_OBJECTSID].name;
We should be using the group_map here.
I think we should also check the attr validity, since this is generic
LDAP code and the LDAP provider doesn't set the option value itself.
+static struct tevent_req *get_user_and_group_send(TALLOC_CTX
*memctx,
+ struct tevent_context *ev,
+ struct sdap_id_ctx *id_ctx,
+ const char *filter_val,
+ int filter_type,
+ int attrs_type)
+{
+ struct tevent_req *req;
+ struct tevent_req *subreq;
+ struct get_user_and_group_state *state;
+ int ret;
+
+ req = tevent_req_create(memctx, &state, struct get_user_and_group_state);
+ if (req == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, ("tevent_req_create failed.\n"));
+ return NULL;
+ }
+
+ state->ev = ev;
+ state->id_ctx = id_ctx;
+ state->dp_error = DP_ERR_FATAL;
+
+ state->op = sdap_id_op_create(state, state->id_ctx->conn_cache);
+ if (!state->op) {
+ DEBUG(2, ("sdap_id_op_create failed\n"));
Nitpick, but can you change the legacy debug level since the patch will
be changed anyway?
+ ret = ENOMEM;
+ goto fail;
+ }
+
+ state->sysdb = state->id_ctx->be->domain->sysdb;
+ state->domain = state->id_ctx->be->domain;
+ state->filter_val = filter_val;
+ state->filter_type = filter_type;
+ state->attrs_type = attrs_type;
+
+ subreq = users_get_send(req, state->ev, state->id_ctx, state->filter_val,
+ state->filter_type, state->attrs_type);
+ if (subreq == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, ("users_get_send failed.\n"));
+ ret = ENOMEM;
+ goto fail;
+ }
+
+ tevent_req_set_callback(subreq, get_user_and_group_users_done, req);
+
+ return req;
+
+fail:
+ tevent_req_error(req, ret);
+ tevent_req_post(req, ev);
+ return req;
+}
+
+static void get_user_and_group_users_done(struct tevent_req *subreq)
+{
+ struct tevent_req *req = tevent_req_callback_data(subreq,
+ struct tevent_req);
+ struct get_user_and_group_state *state = tevent_req_data(req,
+ struct get_user_and_group_state);
+ int ret;
+
+ ret = users_get_recv(subreq, &state->dp_error);
+ talloc_zfree(subreq);
+
+ if (ret == EOK) { /* Matching user found */
+ tevent_req_done(req);
+ return;
+ }
+
Here the code is retrying on all failures, would it be safer to only
retry on ENOENT?
+ subreq = groups_get_send(req, state->ev, state->id_ctx,
state->filter_val,
+ state->filter_type, state->attrs_type);
+ if (subreq == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, ("groups_get_send failed.\n"));
+ tevent_req_error(req, ENOMEM);
+ return;
+ }
+
+ tevent_req_set_callback(subreq, get_user_and_group_groups_done, req);
+}
7:10 a.m.
On Tue, May 14, 2013 at 06:07:05PM +0200, Jakub Hrozek wrote:
On Mon, May 13, 2013 at 12:33:46PM +0200, Sumit Bose wrote:
> Hi,
>
> with these four patches the SID-to-name API can now also be used with
> the AD provider and for local IPA accounts. Since this goes beyond the
> functionality needed by the FreeIPA WebUI I send them in a separate
> series. The patches are also a requirement for using the PAC with the AD
> provider (https://fedorahosted.org/sssd/ticket/1558).
>
> bye,
> Sumit
[PATCH 1/4] IPA: Always initialize ID mapping
> sdap_auth_ctx->service = ipa_options->service->sdap;
> ipa_options->auth_ctx->sdap_auth_ctx = sdap_auth_ctx;
>
> - ret = ipa_get_id_options(ipa_options, bectx->cdb, bectx->conf_path,
> - &sdap_auth_ctx->opts);
> - if (ret != EOK) {
> + if (ipa_options->id == NULL) {
> + ret = EINVAL;
> goto done;
> }
> + sdap_auth_ctx->opts = ipa_options->id;
>
Ack, I would just like to move all assignments to sdap_auth_ctx together
and then assign to ipa_options->auth_ctx for better readability.
Hi Jakub,
thank you for the review. I have fixed all your comments, new versions
attached.
bye,
Sumit
11:58 a.m.
On Wed, May 22, 2013 at 02:10:11PM +0200, Sumit Bose wrote:
On Tue, May 14, 2013 at 06:07:05PM +0200, Jakub Hrozek wrote:
> On Mon, May 13, 2013 at 12:33:46PM +0200, Sumit Bose wrote:
> > Hi,
> >
> > with these four patches the SID-to-name API can now also be used with
> > the AD provider and for local IPA accounts. Since this goes beyond the
> > functionality needed by the FreeIPA WebUI I send them in a separate
> > series. The patches are also a requirement for using the PAC with the AD
> > provider (https://fedorahosted.org/sssd/ticket/1558).
> >
> > bye,
> > Sumit
>
> [PATCH 1/4] IPA: Always initialize ID mapping
> > sdap_auth_ctx->service = ipa_options->service->sdap;
> > ipa_options->auth_ctx->sdap_auth_ctx = sdap_auth_ctx;
> >
> > - ret = ipa_get_id_options(ipa_options, bectx->cdb, bectx->conf_path,
> > - &sdap_auth_ctx->opts);
> > - if (ret != EOK) {
> > + if (ipa_options->id == NULL) {
> > + ret = EINVAL;
> > goto done;
> > }
> > + sdap_auth_ctx->opts = ipa_options->id;
> >
>
> Ack, I would just like to move all assignments to sdap_auth_ctx together
> and then assign to ipa_options->auth_ctx for better readability.
Hi Jakub,
thank you for the review. I have fixed all your comments, new versions
attached.
bye,
Sumit
ACK to all patches.
12:20 p.m.
On Mon, May 27, 2013 at 06:58:23PM +0200, Jakub Hrozek wrote:
On Wed, May 22, 2013 at 02:10:11PM +0200, Sumit Bose wrote:
> On Tue, May 14, 2013 at 06:07:05PM +0200, Jakub Hrozek wrote:
> > On Mon, May 13, 2013 at 12:33:46PM +0200, Sumit Bose wrote:
> > > Hi,
> > >
> > > with these four patches the SID-to-name API can now also be used with
> > > the AD provider and for local IPA accounts. Since this goes beyond the
> > > functionality needed by the FreeIPA WebUI I send them in a separate
> > > series. The patches are also a requirement for using the PAC with the AD
> > > provider (https://fedorahosted.org/sssd/ticket/1558).
> > >
> > > bye,
> > > Sumit
> >
> > [PATCH 1/4] IPA: Always initialize ID mapping
> > > sdap_auth_ctx->service = ipa_options->service->sdap;
> > > ipa_options->auth_ctx->sdap_auth_ctx = sdap_auth_ctx;
> > >
> > > - ret = ipa_get_id_options(ipa_options, bectx->cdb,
bectx->conf_path,
> > > - &sdap_auth_ctx->opts);
> > > - if (ret != EOK) {
> > > + if (ipa_options->id == NULL) {
> > > + ret = EINVAL;
> > > goto done;
> > > }
> > > + sdap_auth_ctx->opts = ipa_options->id;
> > >
> >
> > Ack, I would just like to move all assignments to sdap_auth_ctx together
> > and then assign to ipa_options->auth_ctx for better readability.
>
> Hi Jakub,
>
> thank you for the review. I have fixed all your comments, new versions
> attached.
>
> bye,
> Sumit
ACK to all patches.
Pushed to master.
12:55 p.m.
On Tue, May 14, 2013 at 06:07:05PM +0200, Jakub Hrozek wrote:
On Mon, May 13, 2013 at 12:33:46PM +0200, Sumit Bose wrote:
> + if (el->values[0].length > 2 &&
> + el->values[0].data[0] == 'S' &&
> + el->values[0].data[1] == '-')
> + {
Ack I will just reformat this opening bracket to conform to our coding
guidelines.
By the way I was thinking about coding style with respect to multi-lined
conditional statements and I see the point in using this style, I just
don't like this particular implementation :-) It's sometimes hard to see
where the conditional ends and the code inside the block beings.
I wonder if an empty space (which I saw Ondra using) would help with
cases like this in the future?
if (hungry == true &&
has_food == true) {
eat();
}
7:30 a.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 05/27/2013 01:55 PM, Jakub Hrozek wrote:
On Tue, May 14, 2013 at 06:07:05PM +0200, Jakub Hrozek wrote:
> On Mon, May 13, 2013 at 12:33:46PM +0200, Sumit Bose wrote:
>> + if (el->values[0].length > 2 && +
>> el->values[0].data[0] == 'S' && +
el->values[0].data[1]
>> == '-') + {
>
> Ack I will just reformat this opening bracket to conform to our
> coding guidelines.
By the way I was thinking about coding style with respect to
multi-lined conditional statements and I see the point in using
this style, I just don't like this particular implementation :-)
It's sometimes hard to see where the conditional ends and the code
inside the block beings.
I wonder if an empty space (which I saw Ondra using) would help
with cases like this in the future?
if (hungry == true && has_food == true) {
eat(); }
I *thought* our coding guidelines recommended this format (but I must
have been remembering it from another project somewhere):
if (hungry == true
&& has_food == true
&& !(food_type == apple
&& deliveryperson == witch)) {
eat();
}
i.e. that the conditional operators should lead all the subsequent
lines - acting as a delimiter - and subsequent conditional lines
should be indented. I find this approach to be easily readable as well
as making nested conditionals more clear)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlGko0kACgkQeiVVYja6o6PmuwCfV2xjzVwQbPDh6ici7ojWFaBp
XvsAmgN/7YAZGYS3eM1Z4DiRZiwOEplA
=CVLU
-----END PGP SIGNATURE-----
7:25 a.m.
On Tue, May 28, 2013 at 08:30:01AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 05/27/2013 01:55 PM, Jakub Hrozek wrote:
> On Tue, May 14, 2013 at 06:07:05PM +0200, Jakub Hrozek wrote:
>> On Mon, May 13, 2013 at 12:33:46PM +0200, Sumit Bose wrote:
>>> + if (el->values[0].length > 2 && +
>>> el->values[0].data[0] == 'S' && +
el->values[0].data[1]
>>> == '-') + {
>>
>> Ack I will just reformat this opening bracket to conform to our
>> coding guidelines.
>
> By the way I was thinking about coding style with respect to
> multi-lined conditional statements and I see the point in using
> this style, I just don't like this particular implementation :-)
> It's sometimes hard to see where the conditional ends and the code
> inside the block beings.
>
> I wonder if an empty space (which I saw Ondra using) would help
> with cases like this in the future?
>
> if (hungry == true && has_food == true) {
>
> eat(); }
I *thought* our coding guidelines recommended this format (but I must
have been remembering it from another project somewhere):
if (hungry == true
&& has_food == true
&& !(food_type == apple
&& deliveryperson == witch)) {
eat();
}
i.e. that the conditional operators should lead all the subsequent
lines - acting as a delimiter - and subsequent conditional lines
should be indented. I find this approach to be easily readable as well
as making nested conditionals more clear)
I like this style and I see it was used in some of your code that I was
going through recently. I'll use it as well..
I would like to propose this as amendment for our coding guidelines, but
since they are shared with FreeIPA, would the right place be
freeipa-devel list?
3976
days inactive
3994
days old
sssd-devel@lists.fedorahosted.org
7 comments
3 participants
participants (3)
-
Jakub Hrozek -
Stephen Gallagher -
Sumit Bose