`oauth2` Refresh Token Utilization

[progmem]

The following PR allows for the leveraging of refresh tokens when using OAuth 2.0 authentication. If a refresh token is available, it will be utilized once the current access token has expired.

Given the current best practices for SMART on FHIR EHRs , this seems like it would be useful for anyone utilizing OAuth 2.0 in scenarios where you may have a persistent thread or long-running process (for example, a process taking incoming data from one system and importing it into another via the FHIR APIs).

[radamson]

Thanks for the PR @progmem! The OAuth2 portion of this library needs some work and appreciate the contribution.

I'm not sure if you've looked at PR https://github.com/fhir-crucible/fhir_client/pull/91, but there is some discussion and that might be helpful and some unit tests that might inform how we can fix the existing unit tests or add a couple more. I'd like to get @arscan's thoughts too as he was involved in those discussions.

This PR highlights an issue with the test_client_logs_without_response test in basic_test.rb which is using the OAuth2 implementation without calling set_oauth2_auth as the README suggests. use_oauth2_auth should probably only have a reader accessor to prevent this kind of inconsistency or confusion.

Fixing that test will require stubbing out a few HTTP requests. It would also be nice to have a few small tests with similar stubs to exercise the refresh. https://github.com/fhir-crucible/fhir_client/pull/91 could give a lead on how to get started.

The code overall looks good to me (having to check for @use_oauth2_auth in the method feels a weird since its always checked right above, but I think its a good move to be safe). I am a little worried about what happens if no expires_in lifetime is provided by the authorization server though. That field is optional in SMART and based on the unit tests in the underlying OAuth2 library it looks like expired? will return false if it isn't provided, although I haven't verified it myself. If so, it might be worthwhile to check for expired_in as well as expired? when determining whether to refresh the token.

[progmem]

@radamson I haven't looked at #91 up until this point. Looking over it and reading the last part of your comment, I didn't realize that expires_at (or expires_in) were both hints and not requirements. With that being said, I think in terms of token refreshing it would still be useful to have this strategy:

• If we know the access token is expired (if expires_at or expires_in were provided), we can refresh the token if we know that it's no longer valid.
• If no expires_at or expires_in is present, then we have to make the assumption that "the token does not expire" in the sense that we can't be proactive about it.
• If the token is expired and we have no hinting that it is, then any of the REST calls should encounter a 401 Unauthorized. We can check the response and if it 401s, raise some kind of AuthorizationRequired error.
• If we're using OAuth and we raised AuthorizationRequired, we can rescue it by attempting a token refresh, followed by a retry.
• If AuthorizationRequired is raised again after the retry, we could either re-raise and present the exception to the user, or we can simply model the current behavior which returns the unauthorized response.

I actually wrote a module that handled a similar process, but only on a single method. I should be able to apply it to the base code in a way that would give us this kind of behavior.

[progmem]

@radamson Looking over the code in tandem with the test, there's a bit of a problem in how the test is currently handling things. use_oauth2_auth handles the code flow, but doesn't necessarily mean that requests are going through an OAuth2::AccessToken.

There's existing tests immediately above test_client_logs_without_response that validate whether use_oauth2_auth is being correctly set when set_oauth2_auth, set_basic_auth, etc. is invoked. Because of that, I'm looking at modifying this test to do the following:

• Instead of setting use_oauth2_auth, call either set_oauth2_auth or set_basic_auth. This requires that we add the stub for /token_path/
• Since OAuth2::AccessToken is not RestClient, we need a different set of exceptions to catch.

The alternative to modifying the test would be to simply return on the new refresh_oauth2_session method immediately if the current client is not an OAuth2::AccessToken. However, that would mean that the tests are still faulty.

[radamson]

@progmem Correct that test does have issues which is what I was trying to say with:

This PR highlights an issue with the test_client_logs_without_response test in basic_test.rb which is using the OAuth2 implementation without calling set_oauth2_auth as the README suggests. use_oauth2_auth should probably only have a reader accessor to prevent this kind of inconsistency or confusion.

Fixing that test will require stubbing out a few HTTP requests. It would also be nice to have a few small tests with similar stubs to exercise the refresh. #91 could give a lead on how to get started.

in my above comment.

Good point that we'll have to catch a different set of exceptions though, although that's pretty unfortunate. Maybe eventually we'll want to be able to abstract that away to properly encapsulate the underlying implementations, but that is probably better addressed in its own PR.

[radamson]

I generally agree with the strategy you have proposed for handling expired tokens with the updated considerations for scenarios when the expiration time is not present. I think it would be good to have that as some sort of wrapper around the existing operations so that it can be easily applied to them all while maintaining the separation of concerns between the authorization and restful interactions.

[progmem]

@radamson With regards for a wrapper, I actually started work on a bit of a refactor on the RESTful calls to allow such a situation. My goal in that refactor is to have each of the REST verbs call a method for triaging out to separated modules. The advantage of this is that we can better incorporate the OAuth2 token refresh logic in a way that would allow us to pre-emptively refresh when expires_at or expires_in is present, while also putting us in a better position for handling 401 Unauthorized and the necessary refresh in those cases, too.

In theory this could allow us to throw our own exceptions for testing, for example having FHIR::Rest::Timeout. The only thing that throws a wrench in this is the fact that Faraday (the underlying provider for OAuth2) already does some of this: while RestClient will return a SocketError, Faraday wraps that around its generalized Faraday::ConnectionFailed. We could take a similar approach and raise FHIR::Rest::ConnectionFailed on both timeout and SocketError.

This does present itself as a "scope creep" for this specific PR, which is to handle OAuth2 refresh tokens, but at the same time I believe it could put the codebase in a better position. My inclination is to close this PR, open one up for the separate refactor (which would also address the testing concerns that have been brought up), then open a separate one for the improved OAuth2 refresh methodology. Let me know what your thoughts are on this.

[progmem]

@radamson I managed to finish up the refactoring work that I mentioned above in a separate branch. I essentially separated out the code for handling the REST verb calls into two different subclasses, which are utilized based on whatever @client currently is (RestClient or OAuth2::AccessToken). This also gives flexibility in that other implementations could be added for a different library or set of classes.

I did this as a separate branch primarily because in the context of this PR, this is total scope creep. There's a couple of additional fixes in that work that would fall outside of this PR:

• as mentioned, there's a separation of concerns between FHIR::Client and the code actually invoking the REST calls
• there's a separation of the OAuth2 and non-OAuth2 code for these REST calls
• prior to this refactor, OAuth2 was never utilized for when trying to invoke the REST HEAD verb (:head previously used RestClient explicitly)
• the existing tests never truly handled calls through OAuth2, and weren't written in a way that would compensate for Faraday's exception wrapping

If it seems reasonable, I'd like to close this PR and open a new one based on this new branch of work. I believe this work would also put the codebase in a better position to integrate the work from #91 in, though I'll leave that for discussion.

[radamson]

Awesome @progmem. I haven't had a chance to go through the refactoring work you've done, but this type of work is something we've been talking about for a while so I'm happy to see it!

I'm ok with opening a new PR for that work so we can have a place to host discussion, but it might be a little preemptive to close this one considering the scope creep (although I suppose we could always reopen it). I'll probably have a better sense once I've had a chance to read through you're changes.

One thing that might be worth thinking about in the meantime, and what I'll be thinking about when I'm reviewing, is how we'll ultimately want the API and the implementation to work. I was having a discussion with @arscan and @jawalonoski who brought up some ideas that we may want to at least consider with this refactor: i.e.

• Should we continue to have both the OAuth2 and RestClient implementations under the hood? Right now they are suited to their own use cases, but it requires specialized error handling and more complicated logic as we've seen.
• Should we remove the OAuth2 client and augment or RestClient with OAuth2?
• Should we remove the RestClient and just use the OAuth2 client all the time, if possible?

Simplifying the implementation to only have a single client dependency would help alleviate some of the issues we've been seeing and (hopefully) make future maintenance easier. Thoughts on this @progmem?

I'll take a look at the refactoring work you've done and thanks again for the PRs!

[progmem]

@radamson I'll go ahead and open a new PR for the refactor work at the very least, that way we can invite the discussion about the API and the OAuth2/RestClient implementations into it. I'll post my thoughts to that PR as well, primarily since this current discussion would be better-suited for it.