Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ipa otptoken-sync: return error when sync fails #6472

Closed
wants to merge 2 commits into from

Conversation

flo-renaud
Copy link
Contributor

The command ipa otptoken-sync does not properly handle
errors happening during the synchronization step.

  • Even if an error is detected (such as invalid password
    provided), the command exits with return code = 0. An
    error message is displayed but the exit code should be 1.

  • When an invalid token is provided, the token is not
    synchronized but the error is not reported back to the
    ipa otptoken-sync command.

The first issue can be fixed by raising an exception when
the HTTP response contains an header with an error.
The second issue is fixed by returning LDAP_INVALID_CREDENTIALS
to ldap bind with the sync control if synchronization fails.

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

The command ipa otptoken-sync does not properly handle
errors happening during the synchronization step.

- Even if an error is detected (such as invalid password
provided), the command exits with return code = 0. An
error message is displayed but the exit code should be 1.

- When an invalid token is provided, the token is not
synchronized but the error is not reported back to the
ipa otptoken-sync command.

The first issue can be fixed by raising an exception when
the HTTP response contains an header with an error.
The second issue is fixed by returning LDAP_INVALID_CREDENTIALS
to ldap bind with the sync control if synchronization fails.

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

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
Scenario:  call ipa otptoken-sync with
- an invalid password
- an invalid first token (containing non-digits)
- an invalid sequence of tokens

The test expects a return code = 1.

Related: https://pagure.io/freeipa/issue/9248
Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
@flo-renaud flo-renaud added ipa-4-6 Mark for backport to ipa 4.6 ipa-4-9 Mark for backport to ipa 4.9 ipa-4-10 Mark for backport to ipa 4.10 labels Sep 28, 2022
@rcritten rcritten self-assigned this Sep 28, 2022
@rcritten
Copy link
Contributor

Thanks for the patch, this looks very good. I just have one (pedantic) question.
With otp2/otp3/otp4 are you trying to make this values unambiguous with the otp1 in desynchronized_hotp ?
It took me a second to figure it out and it makes sense so that which value is used is clear, but then again otp1 should be out-of-scope of the test functions. I'm not asking for a change, just curious if I'm understanding your thought process.

@flo-renaud
Copy link
Contributor Author

With otp2/otp3/otp4 are you trying to make this values unambiguous with the otp1 in desynchronized_hotp ?
It took me a second to figure it out and it makes sense so that which value is used is clear, but then again otp1 should be out-of-scope of the test functions. I'm not asking for a change, just curious if I'm understanding your thought process.

Yes, that's exactly my intent: keep track of the sequence for the generated token values.

@rcritten rcritten added the ack Pull Request approved, can be merged label Sep 28, 2022
@rcritten
Copy link
Contributor

Ack, please remove the Temp commit

@flo-renaud
Copy link
Contributor Author

Thanks for the review. Temp commit removed.

@rcritten
Copy link
Contributor

master:

  • f1b2d8a ipa otptoken-sync: return error when sync fails
  • 59db0fa ipatests: add negative test for otptoken-sync

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged ipa-4-6 Mark for backport to ipa 4.6 ipa-4-9 Mark for backport to ipa 4.9 ipa-4-10 Mark for backport to ipa 4.10 pushed Pull Request has already been pushed
Projects
None yet
2 participants