[Freeipa-devel] [PATCH] 0005 Verify otptoken timespan is valid

[Jan Cholasta]

Dne 29.7.2014 v 14:11 David Kupka napsal(a):
> On 07/29/2014 01:21 PM, Jan Cholasta wrote:
>> Dne 24.7.2014 v 10:00 David Kupka napsal(a):
>>>
>>>
>>> On 07/23/2014 05:07 PM, Jan Cholasta wrote:
>>>> Hi,
>>>>
>>>> On 23.7.2014 15:46, David Kupka wrote:
>>>>> https://fedorahosted.org/freeipa/ticket/4244
>>>>
>>>> 1) Use "isinstance(X, Y)" instead of "type(X) is Y".
>>>
>>> Thanks for advice, will try to remember.
>>>>
>>>> 2) When is "type(not_before) is str" or "type(not_after) is str" true?
>>>> The values coming from command options or LDAP should always be
>>>> datetime, never str.
>>>
>>> Actually, it is never true. I don't  know why I thought that there is
>>> such option.
>>>>
>>>> 3) There are some misindentations:
>>>>
>>>> +            raise ValidationError(name='not_after',
>>>> +                                    error='is before not_before!')
>>>>
>>>> +                raise ValidationError(name='not_after',
>>>> +                                    error='is before not_before!')
>>>>
>>>> +                raise ValidationError(name='not_before',
>>>> +                                    error='is after not_after!')
>>>>
>>>> 4) We don't do exclamation marks in errors messages.
>>>
>>> You re right, it's probably better not to shout at customer :)
>>>>
>>>> 5) Generally, when you want to validate command options, you should
>>>> look
>>>> into "options", not "entry_attrs".
>>>>
>>>> 6) This is not right:
>>>>
>>>> +            result = self.api.Command.otptoken_find(ipatokenuniqueid=
>>>> +                entry_attrs.get('ipatokenuniqueid', None))['result']
>>>>
>>>> This is:
>>>>
>>>> +            result =
>>>> self.api.Command.otptoken_show(keys[-1])['result']
>>>
>>> Both works, but Martin explained me why is otptoken_show better and how
>>> it actually works.
>>>>
>>>> Honza
>>>>
>>>
>>
>> Few more nitpicks:
>>
>> 1) You can make _check_interval a little bit shorter by removing a
>> redundant if statement:
>>
>> +def _check_interval(not_before, not_after):
>> +    if not_before and not_after:
>> +        return not_before <= not_after
>> +    return True
> Nice :)
>>
>> 2) Please keep the 2 line space between the last global function and
>> first class in the module.
> It's good to know that there is one less rule to discover.
>>
>> 3) Error messages are for users' eyes, please don't use internal
>> identifiers in them.
> Done.
>>
>> 4) You don't need to check if the result of otptoken_show is empty, it
>> never is.
>>
> Removed. Although, needless check can't hurt.
>

One more thing, sorry I didn't notice it earlier: You must check if 
'ipatokennotbefore' and 'ipatokennotafter' keys are in opttoken-show 
result before using them, otherwise you might end up with:

     ipa: ERROR: non-public: KeyError: 'ipatokennotbefore'
     Traceback (most recent call last):
       File "/usr/lib/python2.7/site-packages/ipaserver/rpcserver.py", 
line 348, in wsgi_execute
         result = self.Command[name](*args, **options)
       File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 
439, in __call__
         ret = self.run(*args, **options)
       File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 
754, in run
         return self.execute(*args, **options)
       File 
"/usr/lib/python2.7/site-packages/ipalib/plugins/baseldap.py", line 
1384, in execute
         *keys, **options)
       File 
"/usr/lib/python2.7/site-packages/ipalib/plugins/otptoken.py", line 356, 
in pre_callback
         notbefore = result['ipatokennotbefore'][0]
     KeyError: 'ipatokennotbefore'

-- 
Jan Cholasta