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

Test error when yubikey hardware not present #2306

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@mrizwan93
Copy link
Contributor

mrizwan93 commented Aug 30, 2018

In order to work with IPA and Yubikey, libyubikey is required.
Before the fix, if yubikey added without having packages, it used to
result in traceback. Now it the exception is handeled properly.
It needs Yubikey hardware to make command successfull. This test
just check of proper error thrown when hardware is not attached.

related ticket : https://pagure.io/freeipa/issue/6979

Signed-off-by: Mohammad Rizwan Yusuf myusuf@redhat.com

@mrizwan93 mrizwan93 force-pushed the mrizwan93:bz-1452081 branch 2 times, most recently from d6f138f to 0c099e7 Aug 30, 2018

@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Aug 30, 2018

False-negatives are likely possible if libyubikey and/or libusb are already installed. I'm not sure what to suggest, perhaps a test skip if one or both of those packages are installed.

@mrizwan93

This comment has been minimized.

Copy link
Contributor Author

mrizwan93 commented Aug 30, 2018

@rcritten Thanks for looking into it.
What if:

  1. uninstall theses packages first if installed already
  2. test
  3. re-install packages if needed by other testsuites
@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Aug 30, 2018

That seems reasonable.

@mrizwan93 mrizwan93 force-pushed the mrizwan93:bz-1452081 branch from 0c099e7 to dbf0519 Aug 31, 2018

@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Sep 7, 2018

You need to move the reinstallation of the packages either into a try/finally or a cleanup in order to restore the files in case of failure (the assert will cause the test to exit).

You conditionally remove packages and unconditionally re-add them. I'd pick one. I don't know enough about how the images are created to know whether uncondition is the way to go, maybe @Rezney can comment.

I'm not sure whether this is failing due to the test expecting the wrong thing or if the added code is not sufficient to correct the issue.

@mrizwan93 mrizwan93 force-pushed the mrizwan93:bz-1452081 branch from dbf0519 to 9124ba1 Dec 3, 2018

@mrizwan93

This comment has been minimized.

Copy link
Contributor Author

mrizwan93 commented Dec 3, 2018

@rcritten @abbra
Does the package name changed for yubikey or it is the error message which I am trying to assert?

2018-12-03 10:57:38,628    DEBUG  [ipatests.pytest_ipa.integration.host.Host.master.cmd52] RUN yum remove -y libyubikey libusb
2018-12-03 10:57:38,628    DEBUG  [ipatests.pytest_ipa.integration.host.Host.master.cmd52] No match for argument: libyubikey
2018-12-03 10:57:38,628    DEBUG  [ipatests.pytest_ipa.integration.host.Host.master.cmd52] No match for argument: libusb
2018-12-03 10:57:38,628    DEBUG  [ipatests.pytest_ipa.integration.host.Host.master.cmd52] Error: No packages marked for removal.
2018-12-03 10:57:38,628    DEBUG  [ipatests.pytest_ipa.integration.host.Host.master.cmd52] Exit code: 1
2018-12-03 10:57:38,628    DEBUG  [ipatests.pytest_ipa.integration.host.Host.master.ParamikoTransport] RUN ['ipa', 'otptoken-add-yubikey', '--owner=admin']
2018-12-03 10:57:38,628    DEBUG  [ipatests.pytest_ipa.integration.host.Host.master.cmd53] RUN ['ipa', 'otptoken-add-yubikey', '--owner=admin']
2018-12-03 10:57:38,628    DEBUG  [ipatests.pytest_ipa.integration.host.Host.master.cmd53] ipa: ERROR: No YubiKey found
2018-12-03 10:57:38,628    DEBUG  [ipatests.pytest_ipa.integration.host.Host.master.cmd53] Exit code: 2

[..]

2018-12-03 10:58:55,493    DEBUG          # try to add yubikey to the user
2018-12-03 10:58:55,493    DEBUG          args = ['ipa', 'otptoken-add-yubikey', '--owner=admin']
2018-12-03 10:58:55,493    DEBUG          cmd = self.master.run_command(args, raiseonerr=False)
2018-12-03 10:58:55,499    DEBUG          assert cmd.returncode != 0
2018-12-03 10:58:55,499    DEBUG          exp_str = ("ipa: ERROR: No backend available. Please install"
2018-12-03 10:58:55,499    DEBUG                     "'libyubikey' and 'libusb' packages first.")
2018-12-03 10:58:55,499    DEBUG  >       assert exp_str in cmd.stderr_text
2018-12-03 10:58:55,499    DEBUG  E       assert "ipa: ERROR: No backend available. Please install'libyubikey' and 'libusb' packages first." in 'ipa: ERROR: No YubiKey found\n'
2018-12-03 10:58:55,499    DEBUG  E        +  where 'ipa: ERROR: No YubiKey found\n' = <pytest_multihost.transport.SSHCommand object at 0x7f8ddbb8e710>.stderr_text```
@abbra

This comment has been minimized.

Copy link
Contributor

abbra commented Dec 3, 2018

First, there is no package installed so nothing to remove. Second, there is no yubikey attached so you get a different error returned and as result a different text message. That different text message is what breaks your text as you did not expect it.

        try:
            yk = yubico.find_yubikey()
        except usb.core.USBError as e:
            raise NotFound(reason="No YubiKey found: %s" % e.strerror)
        except yubico.yubikey.YubiKeyError as e:
            raise NotFound(reason=e.reason)
        except ValueError as e:
            raise NotFound(reason=str(e) + ". Please install 'libyubikey' "
                           "and 'libusb' packages first.")

You need to make sure you are testing the right case.

The package python3-yubico-1.3.2-10.fc29.noarch is installed according to the test logs.

@mrizwan93 mrizwan93 force-pushed the mrizwan93:bz-1452081 branch from 9124ba1 to bf898d3 Dec 3, 2018

@mrizwan93

This comment has been minimized.

Copy link
Contributor Author

mrizwan93 commented Dec 4, 2018

yum remove -y python3-yubico is removing all the freeipa packages (as dependency) as well. So uninstalling python3-yubico is not the solution here.

@abbra , could you guide further?

@abbra

This comment has been minimized.

Copy link
Contributor

abbra commented Dec 4, 2018

@mrizwan93 I don't think you'd be able to test the other exception in PR CI because it is only shown if the hardware is plugged but we have no packages to support it. Otherwise, we always get a generalized YubiKeyError exception that simply says that there is no hardware plugged in.

From FreeIPA point of view, either message is preventing ipa otptoken-add-yubikey to proceed. It means a failure of ipa otptoken-add-yubikey is out test, so if you invert condition and ensure that the test always fails, that would be enough.

@mrizwan93 mrizwan93 force-pushed the mrizwan93:bz-1452081 branch from bf898d3 to c6f71d8 Dec 4, 2018

@mrizwan93 mrizwan93 changed the title Test package installation suggetion being prompt Test error when yubikey hardware not present Dec 4, 2018

@abbra

This comment has been minimized.

Copy link
Contributor

abbra commented Dec 4, 2018

The test looks good now. Please remove the temporary commit.

@abbra abbra added ipa-4-7 and removed needs rebase labels Dec 4, 2018

Test error when yubikey hardware not present
In order to work with IPA and Yubikey, libyubikey is required.
Before the fix, if yubikey added without having packages, it used to
result in traceback. Now it the exception is handeled properly.
It needs Yubikey hardware to make command successfull. This test
just check of proper error thrown when hardware is not attached.

related ticket : https://pagure.io/freeipa/issue/6979

Signed-off-by: Mohammad Rizwan Yusuf <myusuf@redhat.com>

@mrizwan93 mrizwan93 force-pushed the mrizwan93:bz-1452081 branch from c6f71d8 to a4498fd Dec 4, 2018

@abbra abbra added the ack label Dec 4, 2018

@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Dec 4, 2018

This is tagged for backporting to only 4.7 but the related BZ was against 4.6. Should this also be applied to 4.6?

@abbra

This comment has been minimized.

Copy link
Contributor

abbra commented Dec 4, 2018

Yes, I think it can be applied to ipa-4-6 too -- I checked that both a spec file and the client plugin side have the same yubico package references as in newer branches.

@abbra abbra added the ipa-4-6 label Dec 4, 2018

@Tiboris Tiboris added the pushed label Dec 6, 2018

@Tiboris

This comment has been minimized.

Copy link
Member

Tiboris commented Dec 6, 2018

master:

  • e7581cc Test error when yubikey hardware not present
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.