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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to