Validate user input for cert-get-requestdata #590
Conversation
ipaclient/plugins/csrgen.py
Outdated
| @@ -75,10 +75,19 @@ def execute(self, *args, **options): | |||
| util.check_writable_file(options['out']) | |||
|
|
|||
| principal = options.get('principal') | |||
| if principal is None: | |||
| raise errors.InvocationError(message=u"Principal is required") | |||
ipaclient/plugins/csrgen.py
Outdated
| profile_id = options.get('profile_id') | ||
| if profile_id is None: | ||
| profile_id = dogtag.DEFAULT_PROFILE | ||
| helper = options.get('helper') | ||
| if helper is None: | ||
| raise errors.InvocationError(message=u"CSR generation tool is" |
There was a problem hiding this comment.
You are missing a trailing space in this line. You can make the code more readable by indenting the class' arguments:
raise errors.InvocationError(
message=_(u"CSR generation tool is required")
)
ipaclient/plugins/csrgen.py
Outdated
| raise errors.InvocationError(message=u"CSR generation tool is" | ||
| "required") | ||
| if helper.lower() not in ['openssl', 'certutil']: | ||
| raise errors.InvocationError(message=u"Allowed values for CSR " |
|
You are duplicating the list of helpers. It would have been better to have helper defined as a StrEnum. If it isn't too late to change (e.g. no release has shipped with that in the API) then perhaps a separate patch, then you wouldn't need this enforcement at all. |
|
@rcritten I don't know about backward compatibility of changing helper to StrEnum. @MartinBasti @HonzaCholasta Can you please comment on this? |
|
I have no context about how exactly certrequest is supposed to work, but IMO it was done in that way to allow dynamically adding more helpers as plugins, that's why it is Str and not SrEnum, but code doesn't look it may support that. @LiptonB do you remember why Str param was used? @Akasurde Right now there is no backward compatibility because 4.5 will be first release that contains this feature. |
|
I don't think one could really add a new helper without modifying the code, so there's probably no need to allow arbitrary strings. Given that, StrEnum seems appropriate. For the record, #542 removes the |
today is 4.5 release so you have to keep some level of backward compatibility in that PR |
|
@MartinBasti Should I wait for #542 to get merged? |
Fix adds validatation for Principal and CSR generation tool values Fixes https://pagure.io/freeipa/issue/6742 Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
|
Bump for review. |
|
Closing for inactivity. If this PR is still relevant, please re-open it. |
Fix adds validatation for Principal and CSR generation
tool values
Fixes https://pagure.io/freeipa/issue/6742
Signed-off-by: Abhijeet Kasurde akasurde@redhat.com