Authorization Header Specification fixes #110
Conversation
rmmeans
commented
Jan 17, 2017
rmmeans
commented
- The root of this PR stems from this part of the specification: https://tools.ietf.org/html/rfc6749#section-5.2 This section states that in the event a client is authenticating via the “Authorization” header field, then the authorization server MUST respond with a 401 Unauthorized in the event the client does not have access. This validation must occur before any other validations occur since this is an authentication state via a standard header. Only after the authorization has validated - assuming via the Authorization header - can the rest of the processing and validation continue.
- Added some documentation comments on the AuthorizationHandler trait further explaining what a client validation request must do.
- Removed some unnecessary overrides for fetching the clientCrential in the request type case classes as they are unnecessary since they don’t override anything.
- Cleaned up tests to reflect changes for Authorization Header changes, added a few new tests to cover the new case.
* The root of this PR stems from this part of the specification: https://tools.ietf.org/html/rfc6749#section-5.2 This section states that in the event a client is authenticating via the “Authorization” header field, then the authorization server MUST respond with a 401 Unauthorized in the event the client does not have access. This validation MUST occur before any other validations occur since this is an authentication state via a standard header. Only after the authorization has validated - assuming via the Authorization header - can the rest of the processing and validation continue. * Added some documentation comments on the AuthorizationHandler trait further explaining what a client validation request must do. * Removed some unnecessary overrides for fetching the clientCrential in the request type case classes as they are unnecessary since they don’t override anything. * Cleaned up tests to reflect changes for Authorization Header changes, added a few new tests to cover the new case.
|
If this PR looks good and is merged, can a release be cut? Thanks!! |
| @@ -20,7 +20,7 @@ trait GrantHandler { | |||
| */ | |||
| def clientCredentialRequired = true | |||
|
|
|||
| def handleRequest[U](request: AuthorizationRequest, authorizationHandler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] | |||
| def handleRequest[U](validatedClientId: Option[String], request: AuthorizationRequest, authorizationHandler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] | |||
There was a problem hiding this comment.
Is String class enough as type?
If there is possibility to extend validated request, it is better to define as class instead of String.
There was a problem hiding this comment.
I was torn on this whole thing - what I was trying to avoid was the TokenEndpoint already has to validate a client (https://github.com/nulab/scala-oauth2-provider/blob/master/scala-oauth2-core/src/main/scala/scalaoauth2/provider/TokenEndpoint.scala#L13) so when the handleRequest is called - we need some way of telling the request handler that the client is already been validated and this is what the clientId is. If not, that means most of the handlers will just have to ask to validate the client again, even though functionally it was just validated a few lines back in the TokenEndpoint. Suggestions? the naming of things was tripping me up here.
There was a problem hiding this comment.
we could just pass the entire Client type in instead, but none of the current handlers use anything more than the client Id after the client is validated. Perhaps someone may have a custom handler that for some reason needs the client secret.
There was a problem hiding this comment.
AuthorizationRequest has clientCredential instance and GrantHandler#handleRequest method receive validated clientId using AuthorizationRequest instance and AuthorizationRequest instance itself. GrantHandler#handleRequest method requires duplicated variable. I want to avoid it.
I have an idea that solve the problem.
- Change
AuthorizationRequest#clientCredentialinstance todef parseClientCredential: Option[Either[InvalidClient, ClientCredential]method - Change
GrantHandler#handleRequestfirst of arguments tomaybeClientCredential: Option[ClientCredential]fromOption[String]
What do you think?
There was a problem hiding this comment.
Yes - I think this is better 👍 by changing it to parseClientCredential it makes it more clear that any time you call that method you are merely parsing the credential and have no idea of it's validity. I think the variable name in the GrandHandler#handleRequest method should be something more like maybeValidatedClientCred to signify that it is more than just an option of parsed client cred - it has already been validated by the AuthorizationHandler#validateClient provided by the implementor of the library.
I have updated the PR with these changes, let me know if you see something else you would like me to change. Thanks!
|
@rmmeans I just deployed new version as 1.1.0 to maven central repository. |
