On Mon, Apr 02, 2012 at 10:23:37AM +0200, Jan Zeleny wrote:
> Once more, this time rebased on top of current master.
>
> Thanks
> Jan
Patch 0001: Sysdb routines for subdomains
Ack, can you just add a DEBUG message in case sysdb_transaction_commit()
fails?
Done
Patch 0002: Add some utility functions for subdomains
Ack
Patch 0003: Add conn_name to allow different names for domains and
connections
Ack
Patch 0004: Responder part of the subdomain retrieval work
"get_domains_timeout" is not mentioned in neither configAPI nor manpages
+ if (ret == EOK) {
+ DEBUG(SSSDBG_TRACE_FUNC,
+ ("Last call was too recent, nothing to do!\n"));
+ ret = EOK;
^^ this is redundant
+ goto done;
+ } else if (ret != EAGAIN) {
Fixed
Please stick to either goto or return in get_domains_next()
Done
Two review items from the original review are not addressed yet:
There is no explanation of the "hint" parameter (and without one, it's
hard to say what to do with the FIXME) and there is no
sss_dp_get_domains_recv()
The hint is actually described right at the point where it is given as an
argument to dbus call.
sss_dp_get_domains_recv() added
Patch 0005: Modified responder_get_domain()
It is a convention to put a TALLOC_CTX we allocate on as the first
parameter (responder_get_domain())
I know, I mentioned this to Sumit when I was reviewing reviewing the code once
but I forgot to fix it. Fixed now.
I think that it might be useful to store
CONFDB_RESPONDER_GET_DOMAINS_TIMEOUT in rctx rather than load it from
confdb on every request. The case where confdb_get_int either errors out
or the timeout is negative should at least trigger a DEBUG message.
Done and done
As a general question, why are the domain names case sensitive but
subdomain names seem to be case insensitive? For example
check_last_request() is doing a case-sensitive strcmp but
responder_get_domain is doing a strcasecmp.
I wrote this when the same question was on my mind (you can notice the FIXME
right above it). It will be safer to treat all domains and subdomains case
insensitively. Is there any other place where you noticed this?
Also please split the long DEBUG() statement in
responder_get_domain()
into two lines, it doesn't fit on my screen :-)
Done, sorry for the inconvenience ;-)
Patch 0006: Retrieve subdomains if there is a request for fully
qualified user
Looks OK, will just need amendment when sss_dp_get_domains_recv() is
implemented. Currently the patch supports users and netgroups, are there
plans to also support services and the SSH responder? I think we can
leave out the sudo responder for now, sudo doesn't support domains
(yet?). Autofs in theory can send a request for a fully-qualified map,
but it doesn't so the work can be done later, although I would prefer a
ticket to show that we are aware of the issue.
The amendment has been done. About support of another responders - I honestly
don't know. I wasn't aware that a request for fully qualified service/ssh
entity can be given to SSSD. Perhaps Sumit will know more.
Patch 0007: Ask for subdomains in responder in the first request
after
startup
Ack, with the same comment about services and SSH as with patch #6.
Patch 0008: Check sub-domains in nss_cmd_get{pwuid|grgid}_search()
Ack
Patch 0009: data provider: added subdomains
If the bandaid patch I posted on the list the other day is accepted,
there should be a check for the result of the sbus_get_connection() call.
Done
The comment about fast reply in be_get_subdomains() seems misleading
to
me, there is no check for BE_REQ_FAST and the whole code seems like it
can handle online requests only.
Yes, the comment was copied from somewhere else I think. Fixed
As stated in the original review, there needs to be a check for
"!be_cli->bectx->bet_info[BET_SUBDOMAINS].bet_ops != NULL" before
calling be_file_request, otherwise you'll get crashes on domains with no
subdomain support.
Done
Patch 0010: IPA: Add get-domains target
Please use the new DEBUG level macros.
Done
You should use ipa_parse_search_base(), not sdap_parse_search_base()
in the IPA code.
Done
ipa_subdomains_search_base is not documented and not present in the
configAPI.
Done
ipa_subdomains_parse_results(): please use goto here instead of
return
+ if (new_domain_list[c] == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, ("talloc_zero failed.\n"));
+ return ENOMEM;
+ }
Done
The original review suggested to add a ipa_subdomain_map. If you
don't
want to go that route (I would prefer the map, too, there is a map for
every option family in SSSD), please at least #define the attributes
like ipaNTTrustedDomain and ipaNTFlatName to ease changing them later if
needed. Hardcoding attribute names is bad.
Agreed. I don't want to use the map because there is really no reason for
that. But defining constants is a good idea.
Patch 0011: Add domain name to get_account_info request
Ack
Patch 0012: New config options for subdomains
See my reply in the other part of this thread. If you are goig forward
with these options, add them to the configAPI.
Based on previous discussion I removed the part which is setting default
shell. I intend to keep the homedir for two reasons:
I recalled why I created the config option in the first place. It's because
override_homedir can't be set per domain. After your comment yesterday, I
checked to be sure and IMO the man page is incorrect and the override_homedir
only works for responder.
The second reason is that admins might want to separate homedirs for native
users from homedirs for trusted users on some systems.
Patch 0013: Moved expand_homedir_template() from NSS responder to
utility code
Can you change the DEBUG macros to the new format?
Done
Patch 0014: Add s2n extended operation
Old DEBUG macros again. There is a typo in s2n_response_to_attrs, one of
the debug messages says "go [%s]", should be "got [%s]". Otherwise
looks
fine.
Done
Patch 0015: Add ID operations in subdomains
Ack
Patch 0016: Send PAM requests for subdomains to the right provider
Ack
Patch 0017: Basic support for subdomains in auth provider
Ack
Patch 0018: Carry sysdb context and domain info in be_req structure
Ack
Patch 0019: Accept be_req instead if be_ctx in LDAP access provider
Ack
Patch 0020: Detect subdomain request in IPA access provider
Ack
Patch 0021: Utilize sysdb context within be_req in HBAC
Ack
One more thing, I removed the conn_name from patch #2. I intend to keep
new_subdomain() in patch #1 - these two are closely bound together and there
is a circular dependency between them IIRC. Other than this I believe all
patches are compilable.
Thanks
Jan