On Thu, 2012-11-01 at 16:09 -0400, Dmitri Pal wrote: > On 11/01/2012 03:04 PM, Simo Sorce wrote: > > On Thu, 2012-11-01 at 10:53 -0400, Dmitri Pal wrote: > >> On 11/01/2012 09:11 AM, Simo Sorce wrote: > >>> On Thu, 2012-11-01 at 12:03 +0100, Michal Židek wrote: > >>>> On 10/30/2012 04:53 PM, Ondrej Kos wrote: > >>>>> On 10/17/2012 03:28 PM, Michal Židek wrote: > >>>>>> On 10/16/2012 03:45 PM, Stef Walter wrote: > >>>>>>> On 10/16/2012 02:04 PM, Jakub Hrozek wrote: > >>>>>>>> I was wondering for a while whether to change the behaviour directly > >>>>>>>> in > >>>>>>>> confdb_get_string_as_list() but I think the attached patch takes a > >>>>>>>> better > >>>>>>>> approach because the other consumers of confdb_get_string_as_list() > >>>>>>>> do > >>>>>>>> not see any difference between empty and missing parameter. > >>>>>>> Yeah figures. > >>>>>>> > >>>>>>>> The patch works as advertized, there is just one compilation warning: > >>>>>>>> > >>>>>>>> src/providers/simple/simple_access.c: In function > >>>>>>>> 'get_conf_list_or_empty': > >>>>>>>> src/providers/simple/simple_access.c:284:9: warning: unused variable > >>>>>>>> 'r' > >>>>>>>> [-Wunused-variable] > >>>>>>> New patch attached. > >>>>>>> > >>>>>>> Thanks for the review. > >>>>>>> > >>>>>>> Stef > >>>>>>> > >>>>>> Hi Stef, > >>>>>> > >>>>>> your patch solves the problem with *empty* 'simple_allow_users = ', but > >>>>>> it introduces new problem with *nonexistent* simple_allow_users. With > >>>>>> your patch, if no simple_allow_users list was specified, it behaves as > >>>>>> if empty simple_allow_users was provided and denies access to all > >>>>>> users. > >>>>>> > >>>>>> I think this new behaviour could brake existing configurations and it > >>>>>> also differs from the behaviour described in man pages. > >>>>>> > >>>>>> Michal > >>>>>> > >>>>>> _______________________________________________ > >>>>>> sssd-devel mailing list > >>>>>> sssd-devel@lists.fedorahosted.org > >>>>>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > >>>>> I fixed the new problem and prepared patch that applies on top of this > >>>>> one. > >>>>> attached > >>>>> > >>>>> O. > >>>>> > >>>> It is still not working correctly. With your patch *empty* > >>>> simple_allow_users list allows access to everyone (like it should be > >>>> with *nonexistent* list). > >>>> > >>>> The behaviour should be like this: > >>>> simple_allow_users = user1, user2 // allow access to user1 and user2 > >>>> simple_allow_users = // do not allow access to anyone > >>>> /* nothing */ // allow access to everyone > >>>> > >>>> > >>>> I think that the problem is that our .conf file parser ignores empty > >>>> values. Do we have ticket to track this issue? > >>> Nope, and it is not that easy to change IIRC. > >>> > >>> I think for now it would be easier to use a user name of 'none' to > >>> indicate none has access. The only issue would be if a user 'none' > >>> exists. > >>> > >>> Simo. > >>> > >> Is this hard because config DB pre-populates all the default values in > >> the DB and then overwrites it from INI file? > >> Is this the case? > > It's because the DB cannot hold an empty value. > > With the current interface empty value = No value. > > > >> It seems that it should be easy to differentiate whether something is > >> defined and empty value. > > We might be able to do it, maybe we should have a ticket. > > > >> I do not think it is an INI problem. Seems like something is not done > >> correctly in the config DB layer. > >> IMO we should have a ticket to improve that if this is really a problem. > > It is a confdb limitation, but it has never been a problem before, we > > never had to make a difference, and in general I do not like much to use > > these semantics, I feel confused about this syntax if I put on my admin > > hat. > > > > Simo. > > > > May be it should be "$none$" to express none to not ever collide with > allowed names. > Then an empty value would be treated as not defined i.e. everyone allowed. > However in this specific case does "none" really makes sense? Can you > imagine the real world use case that would lead to setting this option > to "none"? Nobody would be able to log into the system is this a use > case we need to worry about?
Probably not, we should just ignore it I guess. Simo. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel