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

[WIP] Prototyped refresh for OAuth2 Bearers #470

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@reactormonk
Copy link
Contributor

reactormonk commented May 3, 2016

Pull Request Checklist

sbt can't find scripts/reformat.

Purpose

This PR adds refresh_token functionality.

Background

I've tried to add some type safety via OAuth2Bearer, It might be a good idea to subclass OAuth2Provider in a similar manner, maybe adding the OAuth2Info via generic.

References

https://tools.ietf.org/html/rfc6749#section-6
Supposedly you can link gitter chats, but I can't get the anchor.

@@ -165,6 +216,34 @@ trait OAuth2Provider extends SocialProvider with OAuth2Constants with Logger {
info => Success(info)
)
}

def maybeRefresh(request: WSRequest, bearer: OAuth2Bearer): Future[(WSResponse, OAuth2Bearer)] = {

This comment has been minimized.

@akkie

akkie May 3, 2016

Member

Please could you explain how this should be called from application code?

This comment has been minimized.

@reactormonk

reactormonk May 3, 2016

Author Contributor
val request = httpLayer.url("some/url/on/the/api").withMethod("post")
val bearer = <from somewhere>
val provider = <from somewhere>
provider.maybeRefresh(request, bearer).map({ case (response, newBearer) =>
      dao.update(newBearer)
})

This comment has been minimized.

@akkie

akkie May 3, 2016

Member

Please see my notes below.

@akkie

This comment has been minimized.

Copy link
Member

akkie commented May 3, 2016

The introduction of the OAuth2Bearer brings a lot of inconsistency to the library. Please be aware that the library doesn't handle OAuth2 only. The library is designed in a way that multiple protocols can be handled over a simple and well shaped interface. Now the OAuth2Bearer brings a lot of trouble. I think you stumbled over the issue with the DAOs.

The library is currently designed in the way that every provider can handle a single AuthInfo implementation. That cannot be easily changed and I see no reason why we should change that, because the refresh can be handled with OAuth2Info because it contains all needed values.

I'm also not satisfied with the introduction to the bearer term itself. I've described the reasons in a mailing list post a while ago. Silhouette should be open to every transport method. Currently this is implemented with the RequestExtractor. The only missing part is the extraction from the the Bearer token scheme.

As noted in the chat, the provider should provide two additional methods:

def refresh[B]()(implicit request: ExtractableRequest[B]): Future[Either[Result, OAuth2Info]]
def refresh(refreshToken: String): Future[Either[Int, OAuth2Info]]

The first method can extract the token from different request parts. It then returns either the OAuth2Info or the status code on error. The second method can be used to retrieve the token without a request. This would follow the same semantic as currently implemented for the access_token retrieval.

Currently your implementation looks to me as it solves only your use-case. But please try to design the changes so that all possible use-cases can be handled.

@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 3, 2016

I can't implement something that depends on anything related to Request or Result, because these describe incoming HTTP requests. This PR would handle outgoing requests.

The bearer isn't a provider, it's a way to typecheck for token_type "Bearer", that's why I switched over to ADT. Maybe I'd better go with a different solution that wouldn't involve ADTs.

@akkie

This comment has been minimized.

Copy link
Member

akkie commented May 3, 2016

val request = httpLayer.url("some/url/on/the/api").withMethod("post")
val bearer = <from somewhere>
val provider = <from somewhere>
provider.maybeRefresh(request, bearer).map({ case (response, newBearer) =>
      dao.update(newBearer)
})

In this case you misuses the library as client for your own endpoint. But the provider should only deal with requests to the authentication endpoints. The name says it: The OAuth2Provider deals with provider(Facebook, Google, ...) related requests and describes the flow handled by the provider. Sorry, but this is something I do not integrate into the library. Such functionality could be implemented into a separate client component. But even this client component shouldn't be part of the core library. Normally you use a API specification like Swagger to describe your endpoints and generate a client for it.

The flow should be as follow. You describe a refresh endpoint in your application. In the action which handles this endpoint you call the OAuth2Provider.refresh method and pass the current request. The provider extracts the refresh token from request, then it handles the refresh flow against the authentication provider and gives you a OAuth2Info back. Now you can store the info or give it back to your client.

@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 3, 2016

Ok, I'll reduce to refresh in that case.

def refresh(refreshToken: String): Future[Either[Int, OAuth2Info]]

Why the Int? Either[WSResponse, OAuth2Info] sounds more useful to me.

def refresh[B]()(implicit request: ExtractableRequest[B]): Future[Either[Result, OAuth2Info]]

I don't see why I would need this method, refreshing is active, it never happens on incoming requests according to the spec IIRC.

@akkie

This comment has been minimized.

Copy link
Member

akkie commented May 3, 2016

Why the Int? Either[WSResponse, OAuth2Info] sounds more useful to me.

The question is: Is it useful to return the status code from the authentication provider? I think it's not necessary to return the code because the logic regarding the return codes is handled inside the refresh method. What do you think?

Anyway, I do not like to declare the WSResponse to the interface because this type is part of the WS Play library which gets replaced in the near future.

I don't see why I would need this method, refreshing is active, it never happens on incoming requests according to the spec IIRC.

It's not really needed, but it thought it would reduce boilerplate code for the flow I've described in my last post.

@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 3, 2016

Anyway, I do not like to declare the WSResponse to the interface because this type is part of the WS Play library which gets replaced in the near future.

How about a case class Response(status: Int, body: String)? I think it would be helpful to have the response from the API.

It's not really needed, but it thought it would reduce boilerplate code for the flow I've described in my last post.

That flow will never happen, refreshing tokens is something active, not via incoming request. Or are you talking about implementing refresh capabilities when your play app is an OAuth provider?

@akkie

This comment has been minimized.

Copy link
Member

akkie commented May 3, 2016

How about a case class Response(status: Int, body: String)? I think it would be helpful to have the response from the API.

What is body?

That flow will never happen, refreshing tokens is something active, not via incoming request. Or are you talking about implementing refresh capabilities when your play app is an OAuth provider?

The app can be a backend for mobile applications as example.

@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 3, 2016

What is body?

The body of the response from the server in case the token refresh fails.

The app can be an API for mobile applications as example.

I can't see how that would work out, I've never done mobile applications.

@akkie

This comment has been minimized.

Copy link
Member

akkie commented May 3, 2016

What about:

class TokenRefreshException(msg: String, code: Int, cause: Throwable = null) extends ProviderException(msg, cause)
@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 3, 2016

Actually, I'm using buildInfo here, so I inherit the error handling from that method.

@reactormonk reactormonk force-pushed the reactormonk:master branch from 9ebeaf1 to 3165804 May 3, 2016

@reactormonk reactormonk force-pushed the reactormonk:master branch from 3165804 to b13b211 May 3, 2016

@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 3, 2016

How can I tell the tests not to look at generatedAt?

@akkie

This comment has been minimized.

Copy link
Member

akkie commented May 3, 2016

How can I tell the tests not to look at generatedAt?

The generatedAt property is not the problem because you can set it in test. The problem is your expired method because it instantiates the Instant class internally. You should pass a Clock instance to your expired method.

def expired(clock: Clock): Boolean
@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 3, 2016

reactormonk added some commits May 3, 2016

@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 3, 2016

Could you help me a bit with the mocking? I've never used mockito before.

@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 4, 2016

Apparently abstract class OAuth2Provider @Inject() (clock: Clock) isn't the right way to go inject Clock - where should I put it in?

@akkie

This comment has been minimized.

Copy link
Member

akkie commented May 4, 2016

Sorry, I'm away until Monday evening.

@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 4, 2016

@reactormonk reactormonk closed this May 5, 2016

@akkie

This comment has been minimized.

Copy link
Member

akkie commented May 9, 2016

Why do you have closed the pull request?

@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 9, 2016

Because I switched over to http4s. But let's finish this up.

@reactormonk reactormonk reopened this May 9, 2016

@reactormonk

This comment has been minimized.

Copy link
Contributor Author

reactormonk commented May 9, 2016

I'm not sure where to @Inject() Clock. I'd guess infoReads needs it, or make that via a separate case class, so Clock is only in OAuth2Provider?

@akkie

This comment has been minimized.

Copy link
Member

akkie commented May 10, 2016

I think you do not need the Clock instance here because you can pass an Instant to the expired method. This is enough.

@akkie

This comment has been minimized.

Copy link
Member

akkie commented Oct 14, 2018

Implemented with minutemen/silhouette#22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.