Implement OAuth refresh token flow

[corvust]

Context:

The platform backend is moving to a new OAuth based authentication flow using offline refresh tokens and access tokens.

Description of the Change:

• Adds an authentication call to retrieve a short-lived access token using the long lived refresh token stored in the configuration (offline token)
• Wraps all API calls to retrieve a new access token if the current one has expired (resulting in a 401 response)

Benefits:

More secure and easier user credential management.

Possible Drawbacks:

Not backwards compatible, all users will be required to generate new tokens and update strawberry fields when the new authentication mechanism is activated.

Related GitHub Issues:

N/A

[codecov]

Codecov Report

Merging #520 (1693576) into master (fd1b1aa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #520   +/-   ##
=======================================
  Coverage   98.03%   98.04%           
=======================================
  Files          71       71           
  Lines        7489     7507   +18     
=======================================
+ Hits         7342     7360   +18     
  Misses        147      147           

Impacted Files	Coverage Δ
strawberryfields/api/connection.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd1b1aa...1693576. Read the comment docs.

[antalszava]

Looks great so far! 💯 Had a few suggestions/questions

[josh146]

Thanks @corvust

[josh146]

Perhaps better to do the following instead of the pop above?

Suggested change

	headers = {**self._headers}
	headers = {**self._headers}
	headers.pop("Authorization", None)

It feels slightly 'safer' than mutating the instance attribute above, even if it has the same effect

[josh146]

This way, if the method fails midway, there are no side effects

[corvust]

My thought here was that since we are in the refresh token method, we already know the existing Authorization header is invalid, so removing it from the instance headers is a housekeeping action before we replace it with a new one. Whether the method fails midway or not, any future requests are still going to fail regardless of if we've popped the header or not.

[josh146]

Is this the only place where the token is stored?

Originally, I was wondering if it made sense to store it locally (temporarily), but I suppose it doesn't matter and is cleaner this way.

[corvust]

Yes the access token is only stored in the Authorization header. I decided against storing it independently since it isn't used anywhere else, is short lived, and a new one can be fetched if it is somehow lost.

[josh146]

👍

[josh146]

Better to have headers=None here, and then inside the method have

headers = headers or {}

[antalszava]

It's worth noting, that due to this call from now on instantiating a Connection object will already try to establish connection with the server.

As users are encouraged to use the higher level RemoteEngine, this would mean that they could face the following:

In [12]: os.environ["SF_API_AUTHENTICATION_TOKEN"] = "WrongToken"

In [13]: sf.RemoteEngine("X8")
---------------------------------------------------------------------------
RequestFailedError                        Traceback (most recent call last)
<ipython-input-13-a1e528990489> in <module>
----> 1 sf.RemoteEngine("X8")

~//strawberryfields/strawberryfields/engine.py in __init__(self, target, connection, backend_options)
    548         self._target = self.DEFAULT_TARGETS.get(target, target)
    549         self._spec = None
--> 550         self._connection = connection or Connection()
    551         self._backend_options = backend_options or {}
    552         self.log = create_logger(__name__)

~//strawberryfields/strawberryfields/api/connection.py in __init__(self, token, host, port, use_ssl, verbose)
     99
    100         self._headers = {"Accept-Version": self.api_version}
--> 101         self._refresh_access_token()
    102
    103         self.log = create_logger(__name__)

~//strawberryfields/strawberryfields/api/connection.py in _refresh_access_token(self)
    334             self._headers["Authorization"] = f"Bearer {access_token}"
    335         else:
--> 336             raise RequestFailedError("Authorization failed for request, please check your token provided.")
    337
    338     def _request(self, method: str, path: str, headers: Dict = None, **kwargs):

RequestFailedError: Authorization failed for request, please check your token provided.

@josh146 would you think that this causes any complications?

Edit: Removed this line in the latest commit. With it being removed, the first request generates the access token. It also made testing easier.

[josh146]

Nice. Thanks for adding these @antalszava!

[antalszava]

[ch3931]

[jswinarton]

Suggested change

	"Authorization failed for request, please check your token provided."
	"Could not retrieve access token. Please check that your API key is correct."

?

[jswinarton]

By the way, you could do response.raise_for_status() instead on line 330, which will raise a requests exception automatically if the status code isn't 200. That could be helpful because you could catch and re-raise the way you're doing here, but chain the exceptions together to get a more detailed traceback that includes the actual status code it failed with.

[antalszava]

Oh @jswinarton, it seems that it would allow other non-error response status codes though (like 201), correct?

[thisac]

Looking great @corvust and @antalszava! As far as I can tell, this seems to do what it's supposed to. 😄 💯

[thisac]

Just for my own understanding; what does this change do? Figured it out!

[thisac]

(minor / thinking aloud) I'm always partial to using del instead of pop (since the latter unnecessarily returns the item as well), but I see that pop might be a cleaner solution, since it works even if the item doesn't exist. 😆

[thisac]

Oh, this explains my question above! 💯