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

[Jan Cholasta]

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.

-- 
Jan Cholasta