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

[Petr Viktorin]

On 07/29/2014 03:58 PM, Jan Cholasta wrote:
> Dne 29.7.2014 v 15:52 David Kupka napsal(a):
>> On 07/29/2014 03:28 PM, Jan Cholasta wrote:
>>> 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'
>>>
>> Sorry for sending fifth wersion of the patch. I must test my patches
>> more carefully.
>>
>
> No problem. I must review more carefully. ACK.
>

Pushed to:
master: 724391a71b018c94aca71b588a24983e228cf2a7
ipa-4-1: 724391a71b018c94aca71b588a24983e228cf2a7
ipa-4-0: 928c87f5c89916e7bb1a6e1e5def45302a2ea8ec

-- 
Petr³