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

Prevent double authentication while using openqaoa-azure #249

Merged
merged 2 commits into from
Jun 14, 2023

Conversation

TerraVenil
Copy link
Contributor

Description

This PR is for solving issue #233.

Checklist

  • I have performed a self-review of my code.
  • I have commented my code and used numpy-style docstrings
  • I have made corresponding updates to the documentation.
  • My changes generate no new warnings
  • I have added/updated tests to make sure bugfix/feature works.
  • New and existing unit tests pass locally with my changes.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

By running tests locally for openqaoa-azure and openqaoa-core.

Copy link
Collaborator

@shahidee44 shahidee44 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solution looks good.

Additional Comments:

  • The proposed solution only accounts for the case where the user has tried to run check_connection on the same device before the device is being passed into the Backend
  • It does not account for any other case, for example, cases where there are incorrect credentials or names. In those cases, the check_connection method is run again for the same device.

Leaving this comment here so we have reference. Approving changes.

@vishal-ph vishal-ph added the unitary_fund A feature supported by an UF grant label Jun 14, 2023
@shahidee44
Copy link
Collaborator

@TerraVenil You will need to update this PR with the latest version of dev so that it can pass the relevant unittests. :)

@shahidee44 shahidee44 merged commit ac9bb97 into entropicalabs:dev Jun 14, 2023
@TerraVenil
Copy link
Contributor Author

TerraVenil commented Jun 14, 2023

It does not account for any other case, for example, cases where there are incorrect credentials or names. In those cases, the check_connection method is run again for the same device.

Hi @shahidee44. Thank you for your comments to PR. Please take a look my comment here #233 (comment), eventually if you have any issues with authentication after timeout (default value is 5 mins) authenticator will throw an error that will stop any further processing. That means it wouldn't execute check_connection twice. But in general it's possible to improve handling of any authentication errors from Azure authenticator by handlingClientAuthenticatorError but from my point of view authenticator throws already user friendly errors.

@shahidee44
Copy link
Collaborator

Thats a good point on Authentication Errors. The intention of the Device object is to catch Exception (s) that occur during the authentication process and just return a boolean based on the success of the authentication. The handling of the error would be done by other layers controlling the Device object. This was done to ensure consistency in error handling across the different Devices.

Also, it seems like I'm made a mistake in my initial comment, the new amendment only authenticates iff the Device object has not tried to authenticate before. The booleans provider_connected and qpu_connected change depending on whether certain parts of the authentication process is successful.

It is then more appropriate to note that:

  • If a Device Object has already tried to be authenticated before passing through to the Backend. Then, there will be no attempt to reauthenticate.

This is an important point to note since in some fringe case where the user authenticates, initializes the Device and runs check_connection, makes some change to the internal attributes to the Device after the fact, then passes that modified Device into the Backend. The Device object will not be reauthenticated.

The main point here would be that we need to let users know that the Device object would need to be recreated or check_connection has to be run again on the same object, if the user is looking to reauthenticate the Device.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unitary_fund A feature supported by an UF grant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants