On Tue, Jun 24, 2014 at 02:35:30PM +0200, Pavel Reichl wrote: > On Thu, 2014-05-22 at 11:30 +0200, Pavel Reichl wrote: > > Hello, > > > > please see attached patches. > > > > I have briefly discussed with Jakub how to handle saving users with uid > > 0 whether to resurrect sysdb_add_fake_user or modify existing fuctions > > for storing users. I decided to add wrapper function around existing > > ones to minimize changes in code which calls them. > > > > Thanks, > > > > Pavel Reichl > > _______________________________________________ > > sssd-devel mailing list > > [email protected] > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > Sorry, there was forgotten //todo comment. Updated patches attached. > > Thanks, > PR
I'm sorry this review took so long. Don't worry about pushing this patch out of 1.12.0 and into 1.12.1. > From c0e570a8381f9abda157ba9dc25554f51773325e Mon Sep 17 00:00:00 2001 > From: Pavel Reichl <[email protected]> > Date: Thu, 22 May 2014 10:37:57 +0100 > Subject: [PATCH 1/2] SYSDB: enable storing urers with uid 0 > > Store users with uid 0 if explicitly asked to. > > Resolves: > https://fedorahosted.org/sssd/ticket/2117 > --- > src/db/sysdb.h | 13 +++++++ > src/db/sysdb_ops.c | 109 > ++++++++++++++++++++++++++++++++++++++++------------- > 2 files changed, 95 insertions(+), 27 deletions(-) > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h > index > 911430eefcbf520ca04f123a3b6f1882dc25876b..79f8b6b04a8e48af8154ae9a5437ec4fba56790a > 100644 > --- a/src/db/sysdb.h > +++ b/src/db/sysdb.h > @@ -658,6 +658,19 @@ int sysdb_store_user(struct sss_domain_info *domain, > uint64_t cache_timeout, > time_t now); > > +int sysdb_store_user_tolerate_uid_zero(struct sss_domain_info *domain, > + const char *name, > + const char *pwd, > + uid_t uid, gid_t gid, > + const char *gecos, > + const char *homedir, > + const char *shell, > + const char *orig_dn, > + struct sysdb_attrs *attrs, > + char **remove_attrs, > + uint64_t cache_timeout, > + time_t now); I'm not really thrilled about the naming conventions. What if the function was named sysdb_store_nonposix_user() and explicitly added or set the SYSDB_UIDNUM attribute to 0? There should also be a unit test along with the new function. > + > int sysdb_store_group(struct sss_domain_info *domain, > const char *name, > gid_t gid, > diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c > index > 4d31cb6a0e83370a2c49a666f9474d93baf430b0..8a3d5094d7f29248b7a64093bee95534e655db1d > 100644 > --- a/src/db/sysdb_ops.c > +++ b/src/db/sysdb_ops.c > @@ -1246,16 +1246,17 @@ done: > > /* =Add-User-Function===================================================== */ > > -int sysdb_add_user(struct sss_domain_info *domain, > - const char *name, > - uid_t uid, gid_t gid, > - const char *gecos, > - const char *homedir, > - const char *shell, > - const char *orig_dn, > - struct sysdb_attrs *attrs, > - int cache_timeout, > - time_t now) > +static int sysdb_add_user_impl(struct sss_domain_info *domain, What does the _impl suffix mean? > + const char *name, > + uid_t uid, gid_t gid, > + const char *gecos, > + const char *homedir, > + const char *shell, > + const char *orig_dn, > + struct sysdb_attrs *attrs, > + int cache_timeout, > + time_t now, > + bool tolererate_uid_zero) Again, I think we should have a common naming, in both patches and I'd like to propose POSIX/non-POSIX. > From 21d66ed6b303837c050fb5f185686abe74265af9 Mon Sep 17 00:00:00 2001 > From: Pavel Reichl <[email protected]> > Date: Thu, 22 May 2014 10:53:15 +0100 > Subject: [PATCH 2/2] LDAP: save non-posix users with ID-mapping off > > Save such users into sysdb with UID and GID set to 0. > Also set isPosix attribute to FALSE. > > Resolves: > https://fedorahosted.org/sssd/ticket/2117 > --- > src/providers/ldap/ldap_id.c | 10 ++++----- > src/providers/ldap/sdap_async_users.c | 41 > +++++++++++++++++++++++++---------- > 2 files changed, 35 insertions(+), 16 deletions(-) > > diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c > index > c788b6bdd6235f5b940d99382b115a2534dbb1d9..10b7a491472aa56540dba34d0ef6303f5cab910b > 100644 > --- a/src/providers/ldap/ldap_id.c > +++ b/src/providers/ldap/ldap_id.c > @@ -197,14 +197,14 @@ struct tevent_req *users_get_send(TALLOC_CTX *memctx, > > ctx->opts->user_map[SDAP_AT_USER_NAME].name, > > ctx->opts->user_map[SDAP_AT_USER_OBJECTSID].name); > } else { > - /* When not ID-mapping, make sure there is a non-NULL UID */ > + /* When not ID-mapping, do not require UID attribute to be present > and > + * if present do not constrain allowed value > + */ > state->filter = talloc_asprintf(state, > - > "(&(%s=%s)(objectclass=%s)(%s=*)(&(%s=*)(!(%s=0))))", > + "(&(%s=%s)(objectclass=%s)(%s=*))", > attr_name, clean_name, > > ctx->opts->user_map[SDAP_OC_USER].name, > - > ctx->opts->user_map[SDAP_AT_USER_NAME].name, > - > ctx->opts->user_map[SDAP_AT_USER_UID].name, > - > ctx->opts->user_map[SDAP_AT_USER_UID].name); > + > ctx->opts->user_map[SDAP_AT_USER_NAME].name); I think this change made the filter a bit too relaxed. I think the proper change would be to allow only entries that don't have a UID but have a SID. Something like (untested, but I hope it illustrates the idea): (|(&(uidNumber=*)(!(uidNumber=0)))(objectSID=*)) Ideally there would be a unit test that would save a basic POSIX user, a basic non-POSIX user and a POSIX user with uidNumber=0. But I'm not sure how easy this would be, our dependencies in the LDAP provider did not have the best granularity. If it was easy to write this unit test, I would prefer it. > } > > talloc_zfree(clean_name); > diff --git a/src/providers/ldap/sdap_async_users.c > b/src/providers/ldap/sdap_async_users.c > index > be0536ef3c3ffaf398c4f2d3e85094d4deb7bd81..ccc4fca06e1bf3cc12fb579935022c3fa7a65b10 > 100644 > --- a/src/providers/ldap/sdap_async_users.c > +++ b/src/providers/ldap/sdap_async_users.c > @@ -142,6 +142,7 @@ int sdap_save_user(TALLOC_CTX *memctx, > char *sid_str; > char *dom_sid_str = NULL; > struct sss_domain_info *subdomain; > + bool is_fake_user = false; I would prefer to use is_posix here. I realize I told you the term 'fake user' myself, but we had used it with a special meaning in the past -- the 'fake' user not only had no UID, but was also marked as expired and only present in the cache until the next request. Kindof like the ghost users we have now. > > DEBUG(SSSDBG_TRACE_FUNC, "Save user\n"); > _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
