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

[David Kupka]

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.

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