openid / AppAuth-Android

Android client SDK for communicating with OAuth 2.0 and OpenID Connect providers.
https://openid.github.io/AppAuth-Android
Apache License 2.0
2.84k stars 884 forks source link

Best practice for integrating with OkHttp (via Retrofit)? #432

Open ratkins opened 5 years ago

ratkins commented 5 years ago

We're working on exchanging an app's custom OAuth2 client implementation with AppAuth. The app uses Retrofit with OkHttp under the hood, and it looks like the appropriate extension point would be implementing okhttp3.Authenticator in order to make calling OAuth-protected endpoints entirely transparent to the caller. But I can't just see how to implement Authenticator in terms of AppAuth's performActionWithFreshTokens(...).

  1. Am I correct about okhttp3.Authenticator being a good extension point for integrating AppAuth with Retrofit/OkHttp?

  2. Can anyone point me to an example of doing this?

gmuellercole commented 5 years ago

Integration with Authenticator may be the right thing to do but our integration with Retrofit is done via the Retrofit Call and has worked well for us.

During startup, we initialize retrofit:

    mRetrofit = new Retrofit.Builder()
            .baseUrl(tUri)
            .addConverterFactory(GsonConverterFactory.create())
            .client(getNoSslCheckOkHttpClient())
            .build();

We define an interface to our ApiGateway:

  // Retrofit call definitions
  public interface ApiGateway {
    @GET("foos")
    @Headers("Accept: application/json")
    Call<FooResponse> readFoos(@Header("Authorization") String accessToken);
    ...
  }

We have a CallExecutor that performs the authenticated request using performActionWithFreshTokens:

  private abstract class RetrofitCallExecutor<T> {
    abstract Call<T> getRetrofitCall(ApiGateway api, String accessToken);

    private final String what;
    private final OnHttpResultListener<T> listener;

    RetrofitCallExecutor(String what, OnHttpResultListener<T> listener) {
      this.what = what;
      this.listener = listener;
    }

    void execute() {
      AuthState as = AuthStateManager.getInstance().getCurrent();
      as.performActionWithFreshTokens(
              mAuthService,
              mAdditionalParams,
              this::hitIt);
    }

    void hitIt(String accessToken, String idToken, AuthorizationException ex) {
      if (ex != null) {
        LoggerLib.e(LoggerLib.Area.AUTH, "Token refresh failed during process of: %s", what);
        return;
      }
      execute(accessToken);
    }

    void execute(String accessToken) {
      if(mRetrofit == null) {
        LoggerLib.w("Network access is not ready aborting attempt to invoke:%s", what);
        return;
      }
      ApiGateway api = mRetrofit.create(ApiGateway.class);
      Call<T> call = getRetrofitCall(api, accessToken);
      CallRunnable<T> callRunnable = new CallRunnable<T>(what, call, listener);
      executor.execute(callRunnable);
    }
  }

The setup for a read looks like:

  private void fetchFoos(final OnHttpResultListener<TopicResponse> listener)
          throws IOException {
    new RetrofitCallExecutor<FooResponse>("fetchFoos", listener) {
      @Override
      Call<TopicResponse> getRetrofitCall(ApiGateway api, String accessToken) {
        return api.readFoos(accessToken);
      }
    }.execute();
  }

The code to trigger the read looks something like:

  public class FooResponseHelper {
    public List<Foo> foos;
    public FooResponseHelper() {
    }
  }

  public void fetchFoosAsync() throws IOException {
    final FooResponseHelper response = new FooResponseHelper();

    final OnHttpResultListener<List<Foo>> listener =
            new AbstractResponseListener<List<Foo>,
                    OnHttpResultListener<FooResponseHelper>>(listener) {
              @Override
              public void onHttpSuccess(List<Foo> result) {
                 ...
              }

              @Override
              public void onHttpFailure(int responseCode) {
                ...
                super.onHttpFailure(responseCode);
              }
            };
    fetchFoos(listener);
  }
ratkins commented 5 years ago

This is roughly the approach we had originally; what I was trying to work around is adding the @Header("Authorization") String accessToken parameter to each declaration in our ApiGateway interface (there are a few of them.)

Since posting this I've made it work, but I needed to include android-retrofuture to get CompletableFuture:

class AppAuthAuthenticator(private val authService: AuthorizationService, private val authState: AuthState) : Authenticator {

    override fun authenticate(route: Route?, response: Response): Request? {

        val future = CompletableFuture<Request?>()

        authState.performActionWithFreshTokens(authService) { accessToken, _, ex ->
            if (ex != null) {
                Log.e("AppAuthAuthenticator", "Failed to authorize = $ex")
            }

            if (response.request().header("Authorization") != null) {
                future.complete(null) // Give up, we've already failed to authenticate.
            }

            val response = response.request().newBuilder()
                    .header("Authorization", "Bearer $accessToken")
                    .build()

            future.complete(response)
        }

        return future.get()
    }

}

There will probably be more to it in practice but that's the shape of it.

alosdev commented 5 years ago

I would use a combination of interceptor, for adding the authorization header and the authenticator to refresh it.

childnode commented 1 year ago

relevant to me: Synchronization should be implemented with LruCache for AuthState, I don't see that synchronized code from performActionWithFreshTokens prevents performTokenRequest is beeing executed on outdated AuthState in parallel use cases

all examples given fail, due to unsynchronized AuthState on asynchronous HTTP stacks (nowadays typically kotlin flows + e.g. okhttp https://square.github.io/okhttp/recipes/#handling-authentication-kt-java)

This is failing by design when multiple calls at the same are invoked with the same AuthState object information, a needed token refresh then

  1. typically forces RaceConditions on updating the AuthState
  2. which then typically causes refresh token reuse count exceeds when token rotation is activated but not swapped correctly on client!
  3. causing client is beeing logged out :doomed:
softsan commented 1 year ago

@childnode i'm also experiencing similar issue of refreshing token with app auth. is there any workaround of this?