[Freeipa-devel] [freeipa PR#590][opened] Validate user input for cert-get-requestdata

[Akasurde]

URL: https://github.com/freeipa/freeipa/pull/590
Author: Akasurde
Title: #590: Validate user input for cert-get-requestdata
Action: opened

PR body:
"""
Fix adds validatation for Principal and CSR generation
tool values

Fixes https://pagure.io/freeipa/issue/6742

Signed-off-by: Abhijeet Kasurde <***@redhat.com>
"""

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

[Akasurde]

URL: https://github.com/freeipa/freeipa/pull/590
Author: Akasurde
Title: #590: Validate user input for cert-get-requestdata
Action: synchronized

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

[rcritten]

URL: https://github.com/freeipa/freeipa/pull/590
Title: #590: Validate user input for cert-get-requestdata

rcritten commented:
"""
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.
"""

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

[Akasurde]

URL: https://github.com/freeipa/freeipa/pull/590
Title: #590: Validate user input for cert-get-requestdata

Akasurde commented:
"""
@rcritten I don't know about backward compatibility of changing helper to StrEnum. @MartinBasti @HonzaCholasta Can you please comment on this?
"""

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

[MartinBasti]

URL: https://github.com/freeipa/freeipa/pull/590
Title: #590: Validate user input for cert-get-requestdata

MartinBasti commented:
"""
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.
"""

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

[LiptonB]

URL: https://github.com/freeipa/freeipa/pull/590
Title: #590: Validate user input for cert-get-requestdata

LiptonB commented:
"""
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, https://github.com/freeipa/freeipa/pull/542 is going to be modified to remove the `helper` parameter of `cert-get-requestdata` entirely, though I haven't had a chance to make the change yet.
"""

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

[LiptonB]

URL: https://github.com/freeipa/freeipa/pull/590
Title: #590: Validate user input for cert-get-requestdata

LiptonB commented:
"""
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, https://github.com/freeipa/freeipa/pull/542 is going to be modified to remove the `helper` parameter of `cert-get-requestdata` entirely, though I haven't had a chance to make the change yet.
"""

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

[MartinBasti]

URL: https://github.com/freeipa/freeipa/pull/590
Title: #590: Validate user input for cert-get-requestdata

MartinBasti commented:
"""

For the record, #542 removes the helper parameter of cert-get-requestdata, and will be modified to remove the concept of different helpers entirely, though I haven't had a chance to make that change yet.

today is 4.5 release so you have to keep some level of backward compatibility in that PR
"""

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

[Akasurde]

URL: https://github.com/freeipa/freeipa/pull/590
Title: #590: Validate user input for cert-get-requestdata

Akasurde commented:
"""
@MartinBasti Should I wait for #542 to get merged?
"""

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

[Akasurde]

URL: https://github.com/freeipa/freeipa/pull/590
Author: Akasurde
Title: #590: Validate user input for cert-get-requestdata
Action: synchronized

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

[Akasurde]

URL: https://github.com/freeipa/freeipa/pull/590
Title: #590: Validate user input for cert-get-requestdata

Akasurde commented:
"""
Bump for review.
"""

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