cert: defer cert-find result post-processing #677
Conversation
ipaserver/plugins/cert.py
Outdated
| @@ -1469,18 +1451,17 @@ def _ldap_search(self, all, raw, pkey_only, no_members, **options): | |||
| try: | |||
| obj = result[key] | |||
| except KeyError: | |||
| obj = self._get_cert_obj(cert, all, raw, pkey_only) | |||
| obj = {} | |||
There was a problem hiding this comment.
I believe this could be a bug right here. Previously, if raw was not specified, the obj variable would contain at least the parse of the certificate (even if pkey_only was specified). Now it's just empty if pkey_only=True.
There was a problem hiding this comment.
Indeed, this is a bug. I added a user certificate. Now, when I do ipa cert-find --pkey-only, the result is:
-----------------------
11 certificates matched
-----------------------
...
-----------------------------
Number of entries returned 11
-----------------------------
but only 10 certificates are shown.
I would also expect that when I specify --pkey-only only primary keys are shown, which is not true but I suspect it's behaved like this for quite some time - only few certificates are displayed with limited information with the previous implementation. The --pkey-only option does not seem to have any effect with this patch, though, except that some certificates are not displayed at all.
ipaserver/plugins/cert.py
Outdated
| @@ -1469,18 +1451,17 @@ def _ldap_search(self, all, raw, pkey_only, no_members, **options): | |||
| try: | |||
| obj = result[key] | |||
| except KeyError: | |||
| obj = self._get_cert_obj(cert, all, raw, pkey_only) | |||
| obj = {} | |||
There was a problem hiding this comment.
Indeed, this is a bug. I added a user certificate. Now, when I do ipa cert-find --pkey-only, the result is:
-----------------------
11 certificates matched
-----------------------
...
-----------------------------
Number of entries returned 11
-----------------------------
but only 10 certificates are shown.
I would also expect that when I specify --pkey-only only primary keys are shown, which is not true but I suspect it's behaved like this for quite some time - only few certificates are displayed with limited information with the previous implementation. The --pkey-only option does not seem to have any effect with this patch, though, except that some certificates are not displayed at all.
|
What worries me the most is that the tests are green even though this is potentially a serious problem. |
|
The patched IPA works better than the current 4.4 and 4.5 branches in terms of options logic, that's good. |
Rather than post-processing the results of each internal search, post-process the combined result. This avoids expensive per-certificate searches when cert-find is executed with the --all option on certificates which won't even be included in the combined result. https://pagure.io/freeipa/issue/6808
|
We may need these changes in 4.5 and 4.4, too since |
|
That might require backporting issue 6564 as well. |
Rather than post-processing the results of each internal search,
post-process the combined result.
This avoids expensive per-certificate searches on certificates which won't
even be included in the combined result when cert-find is executed with the
--all option.
https://pagure.io/freeipa/issue/6808