[Freeipa-devel] [PATCH 0024] Add OTP support to ipalib CLI

[Martin Kosek]

On 12/11/2013 01:24 PM, Jan Cholasta wrote:
> On 14.11.2013 20:23, Nathaniel McCallum wrote:
>> On Wed, 2013-10-30 at 08:57 +0100, Jan Cholasta wrote:
>>> On 8.10.2013 16:35, Nathaniel McCallum wrote:
>>>> On Tue, 2013-10-08 at 09:19 +0200, Jan Cholasta wrote:
>>>>>
>>>>> +class Base32DecodeError(ExecutionError):
>>>>>
>>>>> Is this really necessary? Are we going to add <encoding>DecodeError for
>>>>> every kind of new encoding in IPA? Can't we just have generic
>>>>> DecodeError? (This is not an issue in your patch per se, I'm just
>>>>> wondering if we can do it better in the framework.)
>>>>
>>>> I added the new error due to the existence of a Base64DecodeError. I
>>>> figured changing the existing error to be more generic would break api.
>>>>
>>>> Nathaniel
>>>>
>>>
>>> I think you can use ConversionError instead. I don't see any reason why
>>> base32/64 decoding errors should be special cased like this and would
>>> like to see Base64DecodeError gone.
>>
>> Fixed.
...
> +        # Get the issuer for the URI
> +        issuer = api.env.realm
> +        if owner is not None:
> +            try:
> +                issuer = ldap.get_entry(owner,
> ['krbprincipalname'])['krbprincipalname'][0]
> +            except:
> +                pass
>
> Please use "except PublicError" here, we don't want internal errors to be ignored.

Right. I think what Nathaniel wanted to do is:

except errors.NotFound:
     pass

Bare except-pass is wrong on several levels anyway. For example it also catches 
syntax or interrupt exceptions.

>
>
> +        # Delete all tokens owned by this user
> +        owner = self.api.Object.user.get_primary_key_from_dn(dn)
> +        results = self.api.Command.otptoken_find(ipatokenowner=owner)['result']
> +        for token in results:
> +            token = self.api.Object.otptoken.get_primary_key_from_dn(token['dn'])
> +            self.api.Command.otptoken_del(token)
>
> This should probably be handled by the referint plugin.
>
> Honza
>

I think referint plugin would just remove the attribute containing DN to user, 
not the whole entry. I.e. I think Nathaniel need to do it this way anyway.

Though I think it would be more efficient if we do just one 'otptoken_del' call 
with the list of tokens as primary key to it. All standard LDAPDelete based 
plugins can do that. You may also want to pass continue=True in the list of 
it's options in this case.

Martin