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

[David Kupka]

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.

-- 
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0005-4-Verify-otptoken-timespan-is-valid.patch
Type: text/x-patch
Size: 3559 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140729/d107420b/attachment.bin>