On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
btw (unrelated to these patches) it seems strange to require the user of the sysdb database to connect to the db and then separately initialize subdomains...I would vote for merging that code..
+1
+#include <talloc.h> +#include <popt.h>
+#include "confdb/confdb.h"
+struct sss_tool_ctx {
- struct confdb_ctx *confdb;
- char *confdb_path;
- char *default_domain;
- struct sss_domain_info *domains;
I wonder if the tool_ctx must be open for consumers? Currently only domains is used, maybe we could get away with a getter and an opaque structure? We can always make the structure public in future, but if it's public from the start, then it's easier to add cruft..
I think it is not worth the effort with such simple structure. I am all for opaque structures but here is just no gain in code safety. It would only make code lines bigger.
+enum sss_tool_domain {
- SSS_TOOL_DOMAIN_REQUIRED,
- SSS_TOOL_DOMAIN_OPTIONAL
+};
+int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
struct sss_tool_ctx *tool_ctx,const char *input,enum sss_tool_domain require_domain,Do we need require_domain now that you added the getpwnam?
No, removed.
<refsect1 id='description'>
<title>DESCRIPTION</title><para><command>sss_override</command> enables to create a client-sideview and allows to change selected values of specific userand groups. This change takes effect only on client machine.~~~~~~~~~~~~~~~Here I think we should definitely say local.
</para></refsect1>
We should say that overriding attributes of users and groups only is supported (IIRC we have an RFE somewhere to override netgroups per-client...)
It says "specific user and groups". That says exactly this, I think.
+static errno_t prepare_view(struct sss_domain_info *domain) +{
- char *viewname = NULL;
- errno_t ret;
- ret = sysdb_get_view_name(NULL, domain->sysdb, &viewname);
- if (ret != EOK && ret != ENOENT) {
DEBUG(SSSDBG_OP_FAILURE, "sysdb_get_view_name() failed.\n");return ret;- }
- if (ret == EOK) {
if (!is_default_view(viewname)) {DEBUG(SSSDBG_TRACE_FUNC, "There already exists view %s. ""Only one view is supported. Nothing to do.\n", viewname);This debug level should be more verbose.
ret = EEXIST;goto done;} else if (strcmp(viewname, LOCALVIEW) == 0) {DEBUG(SSSDBG_TRACE_FUNC, "%s view is already present.\n", viewname);ret = EOK;goto done;}- }
- DEBUG(SSSDBG_TRACE_FUNC, "Creating %s view.\n", LOCALVIEW);
- ret = sysdb_update_view_name(domain->sysdb, LOCALVIEW);
- if (ret == EOK) {
printf("SSSD needs to be restarted for the changes to take effect.\n");Hmm, why? Can we instead send a signal to SSSD to pick up the changes? (If yes, then it can be a follow-up patch..)
No, it is not possible to change view on the fly the way views are implemented.
+{
- char *anchor;
- char *safe_dn;
- errno_t ret;
- ret = sysdb_dn_sanitize(mem_ctx, obj_dn, &safe_dn);
Is this sanitization required? Shouldn't a sysdb DN be already sanitized?
Yes, it is. Values in sections of dn are sanitized but we also need to sanitize commas separating sections. Since it is used only as an internal identifier we do not need to care about double escaping.
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "sysdb_dn_sanitize() failed\n");return NULL;- }
- anchor = talloc_asprintf(mem_ctx, ":%s:%s", LOCALVIEW, safe_dn);
- talloc_free(safe_dn);
- return anchor;
+}
enum sysdb_member_type type,const char *name,struct sss_domain_info *domain,struct sss_domain_info *domains,struct sss_domain_info **_new_domain)+{
- TALLOC_CTX *tmp_ctx;
- struct sss_domain_info *dom;
- struct ldb_result *res;
- const char *dn;
- const char *strtype;
- bool check_next;
- errno_t ret;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
return NULL;- }
- /* Ensure that the object is in cache. */
- switch (type) {
- case SYSDB_MEMBER_USER:
if (getpwnam(name) == NULL) {ret = ENOENT;goto done;}break;- case SYSDB_MEMBER_GROUP:
if (getgrnam(name) == NULL) {ret = ENOENT;goto done;}break;- default:
DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);ret = ERR_INTERNAL;goto done;- }
- /* Find domain if it is unknown. */
- if (domain == NULL) {
check_next = true;dom = domains;- } else {
check_next = false;dom = domain;- }
- do {
switch (type) {case SYSDB_MEMBER_USER:DEBUG(SSSDBG_TRACE_FUNC, "Trying to find user %s@%s\n",name, dom->name);ret = sysdb_getpwnam(tmp_ctx, dom, name, &res);strtype = "user";break;case SYSDB_MEMBER_GROUP:DEBUG(SSSDBG_TRACE_FUNC, "Trying to find group %s@%s\n",name, dom->name);ret = sysdb_getgrnam(tmp_ctx, dom, name, &res);strtype = "group";break;default:DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);ret = ERR_INTERNAL;goto done;}if (ret == EOK && res->count == 0) {ret = ENOENT;if (check_next) {dom = dom->next;continue;}}if (ret != EOK) {DEBUG(SSSDBG_CRIT_FAILURE, "Unable to find %s %s@%s [%d]: %s\n",strtype, name, dom->name, ret, sss_strerror(ret));goto done;} else if (res->count != 1) {DEBUG(SSSDBG_CRIT_FAILURE, "More than one %s found?\n", strtype);ret = ERR_INTERNAL;goto done;}check_next = false;- } while (check_next && dom != NULL);
- DEBUG(SSSDBG_TRACE_FUNC, "Domain of %s %s is %s\n",
strtype, name, dom->name);Hmm looks like you should have a check for res validity here, for cases no domain match..
Added a dom validity.
btw it might be nice to split this large function into smaller ones. Maybe the do-while check could be a separate function returning res.
There is no way to split it that would be actually beneficial. There are three blocks: a) beginning - getpwnam b) middle - iteration c) end - linearized dn
If you take away the iteration you can directly return dn from the new function since you don't need res at all so that would also took away the end. And the beginning belongs to the function were iteration is IMHO.
- dn = ldb_dn_get_linearized(res->msgs[0]->dn);
- if (dn == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "ldb_dn_get_linearized() failed.\n");ret = ENOMEM;goto done;- }
- talloc_steal(mem_ctx, dn);
- *_new_domain = dom;
- ret = EOK;
+done:
- talloc_free(tmp_ctx);
- if (ret != EOK) {
return NULL;- }
- return dn;
+}