[freeipa PR#812][opened] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Author: felipevolpone
 Title: #812: Refactoring cert-find to use API call directly instead of using
Action: opened

PR body:
"""
Refactoring cert-find to use API calls directly instead of using raw LDAP search.

Upstream ticket: https://pagure.io/freeipa/issue/6948

I removed the raw LDAP search and used the API directly. In the old code, the call ` self.obj._owners()` returns `service, hots and user`. However, when testing the code, only the service was being used, so I made it only use the service API.

If there another scenario where `user and host` are used, I thought to do something like:

```python
for owner in self.obj._owners():
    api_name = owner.name
    response = api.Command[api_name+'_find'](options[api_name])
    ...  # continues
```
Is that correct?
"""

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/812/head:pr812
git checkout pr812

[freeipa PR#812][comment] Refactoring cert-find to use API call directly instead of using

[simo5]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: Refactoring cert-find to use API call directly instead of using

simo5 commented:
"""
Won't this cause it to not find certificates associated to users ?
Currently that works, this change is not replicating the same functionality of the original code.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-303821229

[freeipa PR#812][comment] Refactoring cert-find to use API call directly instead of using

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: Refactoring cert-find to use API call directly instead of using

HonzaCholasta commented:
"""
@simo5, it will. Same for hosts.

@felipevolpone, one possible correct fix would be to modify `cert._owners` to:
```python
    def _owners(self):
        for obj_name, pkey_name in [('user', None),
                                    ('host', None),
                                    ('service', 'krbprincipalname')]:
            obj = self.api.Object[obj_name]
            if pkey_name is None:
                pkey = obj.primary_key
            else:
                pkey = obj.params[pkey_name]
            yield obj, pkey
```
and replace all occurences of:
```python
    for owner in (...)._owners():
        ... owner.primary_key ...
```
with:
```python
    for owner, owner_pkey in (...)._owners():
        ... owner_pkey ...
```
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-303936108

[freeipa PR#812][comment] Refactoring cert-find to use API call directly instead of using

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: Refactoring cert-find to use API call directly instead of using

stlaz commented:
"""
```
************* Module ipaserver.plugins.cert
ipaserver/plugins/cert.py:1505: [C0303(trailing-whitespace), ] Trailing whitespace)
ipaserver/plugins/cert.py:1506: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1508: [C0303(trailing-whitespace), ] Trailing whitespace)
ipaserver/plugins/cert.py:1512: [C0303(trailing-whitespace), ] Trailing whitespace)
ipaserver/plugins/cert.py:1513: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1514: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1515: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1516: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1517: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1519: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1520: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1516: [E1101(no-member), cert_find._service_search] Instance of 'cert_find' has no '_get_cert_obj' member)
ipaserver/plugins/cert.py:1516: [E0602(undefined-variable), cert_find._service_search] Undefined variable 'raw')
ipaserver/plugins/cert.py:1504: [W0612(unused-variable), cert_find._service_search] Unused variable 'ldap')
```
```
PEP-8 errors:
./ipaserver/plugins/cert.py:1505:1: W293 blank line contains whitespace
./ipaserver/plugins/cert.py:1506:1: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1506:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1507:1: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1508:1: W293 blank line contains whitespace
./ipaserver/plugins/cert.py:1512:1: W293 blank line contains whitespace
./ipaserver/plugins/cert.py:1513:1: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1513:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1514:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1514:2: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1515:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1515:2: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1516:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1516:2: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1517:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1517:2: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1519:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1519:2: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1520:1: W191 indentation contains tabs
```
https://i.imgflip.com/1pol0g.jpg
This does not even compile.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-303937715

[freeipa PR#812][comment] Refactoring cert-find to use API call directly instead of using

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: Refactoring cert-find to use API call directly instead of using

stlaz commented:
"""
```
************* Module ipaserver.plugins.cert
ipaserver/plugins/cert.py:1505: [C0303(trailing-whitespace), ] Trailing whitespace)
ipaserver/plugins/cert.py:1506: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1508: [C0303(trailing-whitespace), ] Trailing whitespace)
ipaserver/plugins/cert.py:1512: [C0303(trailing-whitespace), ] Trailing whitespace)
ipaserver/plugins/cert.py:1513: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1514: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1515: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1516: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1517: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1519: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1520: [W0312(mixed-indentation), ] Found indentation with tabs instead of spaces)
ipaserver/plugins/cert.py:1516: [E1101(no-member), cert_find._service_search] Instance of 'cert_find' has no '_get_cert_obj' member)
ipaserver/plugins/cert.py:1516: [E0602(undefined-variable), cert_find._service_search] Undefined variable 'raw')
ipaserver/plugins/cert.py:1504: [W0612(unused-variable), cert_find._service_search] Unused variable 'ldap')
```
```
PEP-8 errors:
./ipaserver/plugins/cert.py:1505:1: W293 blank line contains whitespace
./ipaserver/plugins/cert.py:1506:1: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1506:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1507:1: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1508:1: W293 blank line contains whitespace
./ipaserver/plugins/cert.py:1512:1: W293 blank line contains whitespace
./ipaserver/plugins/cert.py:1513:1: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1513:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1514:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1514:2: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1515:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1515:2: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1516:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1516:2: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1517:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1517:2: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1519:1: W191 indentation contains tabs
./ipaserver/plugins/cert.py:1519:2: E101 indentation contains mixed spaces and tabs
./ipaserver/plugins/cert.py:1520:1: W191 indentation contains tabs
```
https://i.imgflip.com/1pol0g.jpg
This does not even compile.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-303937715

[freeipa PR#812][edited] [WIP] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Author: felipevolpone
 Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using
Action: edited

Changed field: title
Original value:
"""
Refactoring cert-find to use API call directly instead of using
"""

[freeipa PR#812][comment] [WIP] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using

felipevolpone commented:
"""
Talking with @simo5 on IRC, he proposed to we do not change the whole code, but only fix it doing something like:

```python
filter = ldap.make_filter_from_attr('krbprincipalname', value, rule)
```
in https://github.com/freeipa/freeipa/blob/master/ipaserver/plugins/cert.py#L15...
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-304004141

[freeipa PR#812][comment] [WIP] Refactoring cert-find to use API call directly instead of using

[simo5]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using

simo5 commented:
"""
Ok one thing was in the back of my mind and came up now, we need to keep in mind that krbprincipalname can be multivalued. It won't affect this case (I think) but if we are going to expose some new attribute we need to make sure users of the API are not tricked into thinking it is alays the service's unique name (that's what krbCanonicalName is for).
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-304009324

[freeipa PR#812][comment] [WIP] Refactoring cert-find to use API call directly instead of using

[simo5]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using

simo5 commented:
"""
So Iam for the very localized change still (to be clear)
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-304009517

[freeipa PR#812][synchronized] [WIP] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Author: felipevolpone
 Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/812/head:pr812
git checkout pr812

[freeipa PR#812][comment] [WIP] Refactoring cert-find to use API call directly instead of using

[martbab]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using

martbab commented:
"""
Remember taht you have to use 'exact=False' in the filter to perform substring search for krbPrincipalName given the fact that (except for services) the principal is constructed from primary key by appending realm (and prepending `host/` in the case of hosts). This, however, opens a range of possibilities for new bug to creep in (considering 'tuser' is the owner but we have 'tuser1' and 'tuser2' in LDAP, what will your search filter return?).

That's why I think this is not correct solution given we currently reference owners by primary keys and not by principals (krbPrincipalName != primary key in most cases except services without krbCanonicalName attribute). I am more inclined to @HonzaCholasta's solution as it seems cleaner to me. An alternative is to report principals as cert owners, which will break API, however.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-304304587

[freeipa PR#812][comment] [WIP] Refactoring cert-find to use API call directly instead of using

[martbab]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using

martbab commented:
"""
Remember taht you have to use 'exact=False' in the filter to perform substring search for krbPrincipalName given the fact that (except for services) the principal is constructed from primary key by appending realm (and prepending `host/` in the case of hosts). This, however, opens a range of possibilities for new bug to creep in (considering 'tuser' is the owner but we have 'tuser1' and 'tuser2' in LDAP, what will your search filter return?).

That's why I think this is not correct solution given we currently reference owners by primary keys and not by principals (krbPrincipalName != primary key in most cases except services without krbCanonicalName attribute). I am more inclined to @HonzaCholasta's solution as it seems cleaner to me. An alternative is to report principals as cert owners, which will break API, however.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-304304587

[freeipa PR#812][comment] [WIP] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using

felipevolpone commented:
"""
@HonzaCholasta thanks for the help, but the idea isn't avoid calling the ldap directly, and instead of that call the APIs?

If we change _only_ the `_owners` method, we'll still have ldap calls here: https://github.com/freeipa/freeipa/blob/master/ipaserver/plugins/cert.py#L15... . Am I missing something?
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-304978593

[freeipa PR#812][comment] [WIP] Refactoring cert-find to use API call directly instead of using

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using

HonzaCholasta commented:
"""
@felipevolpone, that is a bad idea. Calling the API instead of doing a direct LDAP search would degrade performace (currently everything is done in a single LDAP search, with API calls it will be *at least* one LDAP search per owner class) and offers less flexibility (the current code allows you to find *any* LDAP entry which refers to a certificate, with API calls you are limited to whatever is defined in the API).

The PR currently breaks the `--user` and `--host` options, because they no longer expect a user name and host name, but principal names (as @martbab already pointed out).
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-305090643

[freeipa PR#812][synchronized] [WIP] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Author: felipevolpone
 Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/812/head:pr812
git checkout pr812

[freeipa PR#812][comment] [WIP] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using

felipevolpone commented:
"""
@HonzaCholasta thank you for the explanation, I misunderstood the ticket title. I did the changes that you suggested. 
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-305210568

[freeipa PR#812][synchronized] [WIP] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Author: felipevolpone
 Title: #812: [WIP] Refactoring cert-find to use API call directly instead of using
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/812/head:pr812
git checkout pr812

[freeipa PR#812][edited] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Author: felipevolpone
 Title: #812: Refactoring cert-find to use API call directly instead of using
Action: edited

Changed field: title
Original value:
"""
[WIP] Refactoring cert-find to use API call directly instead of using
"""

[freeipa PR#812][synchronized] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Author: felipevolpone
 Title: #812: Refactoring cert-find to use API call directly instead of using
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/812/head:pr812
git checkout pr812

[freeipa PR#812][comment] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: Refactoring cert-find to use API call directly instead of using

felipevolpone commented:
"""
Done. @frasertweedale if there is something wrong with the commit message, please tell me.
Thanks for reviewing :) 
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-305600312

[freeipa PR#812][synchronized] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Author: felipevolpone
 Title: #812: Refactoring cert-find to use API call directly instead of using
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/812/head:pr812
git checkout pr812

[freeipa PR#812][comment] Refactoring cert-find to use API call directly instead of using

[felipevolpone]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: Refactoring cert-find to use API call directly instead of using

felipevolpone commented:
"""
Done. @frasertweedale if there is something wrong with the commit message, please tell me.
Thanks for reviewing :) 
"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-305600312

[freeipa PR#812][+ack] Refactoring cert-find to use API call directly instead of using

[frasertweedale]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: Refactoring cert-find to use API call directly instead of using

Label: +ack

[freeipa PR#812][+pushed] Refactoring cert-find to use API call directly instead of using

[MartinBasti]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: Refactoring cert-find to use API call directly instead of using

Label: +pushed

[freeipa PR#812][comment] Refactoring cert-find to use API call directly instead of using

[MartinBasti]

URL: https://github.com/freeipa/freeipa/pull/812
Title: #812: Refactoring cert-find to use API call directly instead of using

MartinBasti commented:
"""
master:

* 44bd5e358b027f8956b730f250854efb5087f05e Changing cert-find to do not use only primary key to search in LDAP.

ipa-4-5:

* df1276e9daf9ee8db05538f47706855cb3d11e01 Changing cert-find to do not use only primary key to search in LDAP.

"""

See the full comment at https://github.com/freeipa/freeipa/pull/812#issuecomment-305811098

[freeipa PR#812][closed] Refactoring cert-find to use API call directly instead of using

[MartinBasti]

URL: https://github.com/freeipa/freeipa/pull/812
Author: felipevolpone
 Title: #812: Refactoring cert-find to use API call directly instead of using
Action: closed

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/812/head:pr812
git checkout pr812