Implement authenticator

[grahamsmith]

An attempt to solve #47

Inspired by OkHTTP's authenticator

The idea is to provide a reactive authentication in the event that an auth challenge is raised. It returns a nullable Request that contains a possible update to the original Request to satisfy the authentication challenge.

e.g. a request is made to a server that responses with a 401. The authenticator can be implemented however needed to update the original request, such as using a refresh token, then pass a new request back to be executed.

[grahamsmith]

I can add some tests (with some help) if the general idea is accepted.

[JEuler]

Hi! Cool! Could you show please the example of usage? So, for example, I have refresh_token and refreshToken endpoint, how I should implement the Authenticator?

[lejard-h]

Not sure to understand why this would allow retrying request, maybe the name Authenticator is confusing. I am not familiar with okHTTP, do you have an example ?

[grahamsmith]

Basic example:

class MyAuthenticator extends Authenticator {
  
  @override
  FutureOr<Request> authenticate(Request request, Response<dynamic> response) async{
    
    if(response.statusCode == 401) {
      
      var newToken = await refreshToken();
      
      var updatedHeaders = Map.of(request.headers);

      updatedHeaders.update('Authorization', (value) => newToken, ifAbsent: () => newToken);
      
      return request.copyWith(headers: updatedHeaders);
    } else {
      return null;
    }
  }
  
  Future<String> refreshToken() async{
    //refresh the accessToken using refreshToken however needed....
    //this could be done either via http client or a ChopperService or a repository could be a dependency, this approach is intentionally not opinionated about how this works.
  }
}

There are strategies around handling infinite loops. Some include via headers, checking paths or counting retries and many more options. This implementation is designed to be un-opinionated as everyone's implementations will have their own nuances.

I think the name makes sense.

Plenty of tutorials for OkHttp that are relatable. I would imagine a lot of mobile devs moving to Flutter will reference it due to its popularity (over 39,000 stars on Github).

https://github.com/square/okhttp/blob/480c20e46bb1745e280e42607bbcc73b2c953d97/okhttp/src/main/kotlin/okhttp3/Authenticator.kt
https://square.github.io/okhttp/4.x/okhttp/okhttp3/-authenticator/
https://medium.com/@olempico/okhttp-authenticator-lets-be-secure-in-an-elegant-way-3bf6096752dc
https://objectpartners.com/2018/06/08/okhttp-authenticator-selectively-reauthorizing-requests/
https://blog.coinbase.com/okhttp-oauth-token-refreshes-b598f55dd3b2
https://www.lordcodes.com/articles/authorization-of-web-requests-for-okhttp-and-retrofit
https://stackoverflow.com/questions/22490057/android-okhttp-with-basic-authentication
http://sangsoonam.github.io/2019/03/06/okhttp-how-to-refresh-access-token-efficiently.html
https://medium.com/tiendeo-tech/android-refreshing-token-proactively-with-okhttp-interceptors-e7b90c47f5e7
https://medium.com/@sandeeptengale/problem-solved-2-access-token-refresh-with-okhttp-authenticator-5ccb798ede70

[grahamsmith]

I think there is also some work to do around not invoking the interceptors again. I wanted to start simple before getting into this and see if there was buy in.

edit: I forgot to mention that this change should be non breaking so it would not cause update headaches if people do not want to update 👍

[stewemetal]

This is a good approach IMHO. Users of Chopper can utilize the Request/Response API to implement retry with authentication logic without depending on http directly.

It seems to be in line with OkHttp's solution linked above.

I'll do a review on the current implementation.

If this is merged, documentation should be updated with explanation on how the Authenticator should be used with example(s).

[grahamsmith]

@stewemetal - Is there anything I can do to help push this along? I really could do with this functionality in my own project. Noticed it was added to the milestone which is cool 👍

[JEuler]

I think this can be approved, as I saw your example. "Infinite" retries is a local problem, that can be solved per project. Looking cool.

But, @stewemetal, please also have a look! :)

[stewemetal]

This description fits the authenticate method rather than the whole class. Also, I would rephrase it like this:

/// This method should return a [Request] that includes credentials to satisfy an authentication challenge received in
/// [response]. It should return `null` if the challenge cannot be satisfied.

[stewemetal]

So the class needs a proper, more generic description too. I'd even go as far as putting a link to OkHttp's similar mechanic (like you did in the PR).

[stewemetal]

"handles provides" -> "can provide"

[stewemetal]

This empty line is unnecessary.

[stewemetal]

I've done a quick review on the PR.

@grahamsmith Please fix the mentioned things. 😉

[stewemetal]

Also, I'll find a place for the example implementation in the updated docs, it looks good. 😄

[stewemetal]

@stewemetal - Is there anything I can do to help push this along? I really could do with this functionality in my own project. Noticed it was added to the milestone which is cool 👍

I'd like to consult with @lejard-h and @JEuler soon on the topic of release management and releasing 3.0.7 with revamped docs and guides. As the Authenticator mechanic is a non-breaking change, it can indeed be part of 3.0.7 (hence the inclusion to the milestone).

[JEuler]

As I said, I feel like it is a cool, non-breaking, additional mechanic, so, no problem with adding it to the upcoming release 👍

[grahamsmith]

@stewemetal happy to make the changes. For future PRs I think it would be quicker to just make the changes (I believe maintainers are allowed to contribute back to my fork) rather than me copy paste them in.

[JEuler]

@grahamsmith I've added changes requested from @stewemetal :) Without the description.

[grahamsmith]

Amazing thanks! I've been having a tough week so that's very much appreciated 🎉😄

[JEuler]

@stewemetal Do you want us to make this change with the description? Or we are okay to merge it as it is? :)

[stewemetal]

I think it's OK, I'll approve it. 😄
I'll add the extra description in another docs update PR.

[Aqluse]

You guys sure that this authenticator != null check won't let us fall into an endless recursion? Because I seem to get the one

[grahamsmith]

@Aqluse yes someone has broken the implementation I wrote.

It was originally

var updatedRequest = authenticator.authenticate(request, res);

      if(updatedRequest != null) {
        res = await send(updatedRequest);
      }

@stewemetal and friends this will now result in an infinite loop with current implementation.

[JEuler]

Let's fix it then :)

[grahamsmith]

I'm on holiday at the moment so will leave to those with Dev laptops and reliable internet.

[stewemetal]

Thanks for pointing this out! This implementation has gone awkward at some point during the null safety migration.

Would this change fix your use-case? It breaks the current Authenticator implementations, but it's just a first try on a quick fix:

abstract class Authenticator {
  FutureOr<Request?> authenticate(Request request, Response response);
}


// send()
...
if (authenticator != null) {
  var updatedRequest = await authenticator!.authenticate(request, res);

  if (updatedRequest != null) {
    res = await send(updatedRequest);
  }
}
...

[grahamsmith]

Looks good

[stewemetal]

Fixed in #241