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(a)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.
I also thought it'd be quite useless to have the list set, but nobody in
it, resulting in no one being able to log in, but as we discussed it
off-list, it appeared it's by design.
So how should I now proceed with the fix? What is the behaviour we
consider right?
O.
--
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech
phone: +420-532-294-558
cell: +420-736-417-909
ext: 82-62558
loc: 1013 Brno 1 office
irc: okos @ #brno