Refactor OAuth 2.0 modules to easily implement extension grants #5476

ikhoon · 2024-02-27T15:54:21Z

Motivation:

Currently, the access token grant flow is tightly coupled with AccessTokenRequest. As a result, we have to implement OAuth2XXXGrant, OAuth2XXXGrantBuilder, and XXXTokenRequest to implement a new grant flow.

An extension grant such as JWT is slightly different from the existing access token grant flow. The only difference is in body parameters. It is not practical to create three classes and use internal API for that.

To resolve the problem, I designed new APIs and refactored the existing OAuth2 module so that users could easily implement a new extension grant.

API design note:

OAuth2Request: Represents an OAuth 2.0 request that can be serialized to HttpRequest.
AccessTokenRequest: Provides body parameters and client authentication to obtain an access token.
- This API will be used as an extension point to support new grants.
- The implementations of AccessTokenRequest
  - ClientCredentialsAccessTokenRequest
  - ResourceOwnerPasswordAccessTokenRequest
  - JsonWebTokenAccessTokenRequest
ClientAuthentication: Provides client authentication for the OAuth 2.0 requests. OAuth2Request has ClientAuthentication as an optional value.
- The interface is a drop-in replacement for ClientAuthorization which was a final class that does not allow customization.
- Additionally, as per OAuth 2.0 spec, Client Authentication is a correct expession.
OAuth2ResponseHandler: Handles OAuth 2.0 responses.
- This interface would be useful if users want to handle responses with a custom logic.
TokenOperationRequest: Provides body parameters for token revocation and token introspection.
OAuth2AuthorizationGrantBuilder: A unified builder to fluently create OAuth2AuthorizationGrant.
- This will replace both OAuth2ResourceOwnerPasswordCredentialsGrantBuilder and OAuth2ClientCredentialsGrantBuilder.
- No additional builder is required to support new grant types.

Example:

// Client credentials grant
AccessTokenRequest accessTokenRequest =
  AccessTokenRequest.ofClientCredentials("test_client", "client_secret");
OAuth2AuthorizationGrant grant = 
  OAuth2AuthorizationGrant
    .builder(authClient, "/token")
    .accessTokenRequest(accessTokenRequest)
    .build();

// Resource owner password grant
AccessTokenRequest accessTokenRequest =
  AccessTokenRequest.ofResourceOwnerPassword("user_name", "user_password");
OAuth2AuthorizationGrant grant = 
  OAuth2AuthorizationGrant
    .builder(authClient, "/token")
    .accessTokenRequest(accessTokenRequest)
    .build();

As you can see, the only difference is how you create AccessTokenRequest. The rest of the process will use the same API regardless of the grant type used.

Modifications:

Removed the following internal classes that are no longer used.
- internal.client...AbstractAccessTokenRequest
- internal.client...ClientCredentialsTokenRequest
- internal.client...RefreshAccessTokenRequest
- internal.client...ResourceOwnerPasswordCredentialsTokenRequest
- internal.common...AbstractTokenOperationRequest
- internal.common...TokenRevocationRequest
- internal.common...TokenIntrospectionRequest
Added AccessTokenRequest and their implementations
- The main difference from the old API is that the new AccessTokenRequest is an immutable class that only provides body parameters and client authentication.
- AccessTokenRequest does not know how the request is processed. Implementations of OAuth2AuthorizationGrant and OAuth2ResponseHandler will take responsibility for it.
- Implementations:
  - ClientCredentialsAccessTokenRequest
  - ResourceOwnerPasswordAccessTokenRequest
  - RefreshAccessTokenRequest
  - JsonWebTokenAccessTokenRequest (NEW)
Deprecated the legacy APIs
- OAuth2ResourceOwnerPasswordCredentialsGrant
- OAuth2ResourceOwnerPasswordCredentialsGrantBuilder
- OAuth2ClientCredentialsGrant
- OAuth2ClientCredentialsGrantBuilder
- There are no breaking changes in the public API. The old APIs will delegate logic to the new API.
- As they are unstable APIs, we may delete them after releasing some new minor versions.
Added ClientAuthentication that provides authentication information to Authorization header or body parameters.
- Deprecated ClientAuthorization and internally delegated to ClientAuthentication.
- Implementations:
  - ClientPasswordClientAuthentication
  - AnyAuthorizationSchemeClientAuthentication
  - JsonWebTokenClientAuthentication (NEW)
Added TokenOperationRequest to replace TokenRevocationRequest and TokenIntrospectionRequest.

Result:

You can now easily implement OAuth 2.0 Extension Grants by extending AccessTokenRequest.
You can now use JSON Web Token to obtain OAuth 2.0 access tokens.

Motivation: Currently, access token grant flow is tightly coupled with AccessTokenRequest. As a result, we have to implement `OAuth2XXXGrant`, `OAuth2XXXGrantBuilder` and `XXXTokenRequest` to implement a new grant flow. An extension grant such as JWT is slightly different from the exisiting access token grant flow. The only difference is in body parameters. It is not pracitial to create three classes and use internal API for that. To resolve the problem, I designed new APIs and refactored the existing OAuth2 module so that users can easily implement a new extention grant. API design note: - `OAuth2Request`: Represents an OAuth 2.0 request that can be serialized to `HttpRequest`. - `AccessTokenRequest`: Provides body parameters and client authentication to obtain an access token. - This API will be used as an extension point to support new grants. - The implmentations of `AccessTokenRequest` - `ClientCredentialsAccessTokenRequest` - `ResourceOwnerPasswordAccessTokenRequest` - `JsonWebTokenAccessTokenRequest` - `ClientAuthentication`: Provides a client authentication for the OAuth 2.0 requests. `OAuth2Request` has `ClientAuthentication` as an optional value. - The interface is a drop-in replacement for `ClientAuthorization` which was a final class that does not allow customization. - Additionally, as per OAuth 2.0 spec, Client Authentication is a correct expession. - `OAuth2ResponseHandler`: Handles OAuth 2.0 responses. - This interface would be useful if users want to handle responses with a custom logic. - `TokenOperationRequest`: Provides body parameters for token revocation and token introspection. Modifications: - TBU

github-actions · 2024-02-27T16:09:21Z

🔍 Build Scan® (commit: `b60a9f7`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/aahg4zn5noo26
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/bbsrarhbliezc
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/v7tvyq4adejsa
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/3ugarc5p3gp46
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/5bvsbbkbjwvt4
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/x4jme4supvso4
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/2nhnhxmx2rnau
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/7glcab3ineoei

ikhoon · 2024-02-28T02:41:42Z

@max904-github Based on your work, I improved the API while maintaining the internal implementation. If available, please give me a review. 🙇‍♂️

minwoox

Looks much better now. 👍 👍 👍
Left minor comments and questions.

oauth2/src/main/java/com/linecorp/armeria/client/auth/oauth2/AbstractAccessTokenRequest.java

oauth2/src/main/java/com/linecorp/armeria/client/auth/oauth2/AccessTokenRequest.java

oauth2/src/main/java/com/linecorp/armeria/common/auth/oauth2/OAuth2Request.java

oauth2/src/main/java/com/linecorp/armeria/common/auth/oauth2/TokenOperationRequest.java

oauth2/src/main/java/com/linecorp/armeria/common/auth/oauth2/TokenRevocationBuilder.java

minwoox · 2024-03-26T09:19:59Z

...h2/src/main/java/com/linecorp/armeria/internal/common/auth/oauth2/AbstractOAuth2Request.java

        }
+        return bodyParams = OAuth2Request.super.bodyParams();


bodyParams does not contain clientAuthentication if bodyParams() is called before asHttpRequest is called.
What do you think about raising an exception in such a situation?

Refactored the code a bit to produce bodyParams correctly.

…ccessTokenRequest.java Co-authored-by: minux <songmw725@gmail.com>

codecov · 2024-03-26T11:10:25Z

Codecov Report

Attention: Patch coverage is 60.98131% with 167 lines in your changes are missing coverage. Please review.

Project coverage is 74.06%. Comparing base (0b20455) to head (eaaf517).
Report is 61 commits behind head on main.

❗ Current head eaaf517 differs from pull request most recent head b60a9f7. Consider uploading reports for the commit b60a9f7 to get more accurate results

Files	Patch %	Lines
...ria/common/auth/oauth2/TokenRevocationBuilder.java	0.00%	22 Missing ⚠️
...client/auth/oauth2/AbstractAccessTokenRequest.java	56.41%	14 Missing and 3 partials ⚠️
...mmon/auth/oauth2/DefaultTokenOperationRequest.java	29.16%	16 Missing and 1 partial ⚠️
...mon/auth/oauth2/AbstractOAuth2ResponseHandler.java	38.46%	13 Missing and 3 partials ⚠️
...rp/armeria/common/auth/oauth2/TokenRevocation.java	0.00%	14 Missing ⚠️
...auth2/ResourceOwnerPasswordAccessTokenRequest.java	38.88%	11 Missing ⚠️
...h2/AnyAuthorizationSchemeClientAuthentication.java	35.29%	11 Missing ⚠️
...nt/auth/oauth2/JsonWebTokenAccessTokenRequest.java	33.33%	10 Missing ⚠️
.../client/auth/oauth2/RefreshAccessTokenRequest.java	33.33%	10 Missing ⚠️
...uth/oauth2/ClientPasswordClientAuthentication.java	64.28%	7 Missing and 3 partials ⚠️
... and 8 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5476      +/-   ##
============================================
+ Coverage     74.02%   74.06%   +0.03%     
- Complexity    20781    21047     +266     
============================================
  Files          1801     1832      +31     
  Lines         76530    77493     +963     
  Branches       9749     9877     +128     
============================================
+ Hits          56654    57394     +740     
- Misses        15264    15421     +157     
- Partials       4612     4678      +66

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

oauth2/src/main/java/com/linecorp/armeria/common/auth/oauth2/ClientAuthentication.java

...h2/src/main/java/com/linecorp/armeria/internal/common/auth/oauth2/AbstractOAuth2Request.java

minwoox

LGTM once @trustin's comment is addressed.
Thanks a lot, @ikhoon! 👍 👍 👍

jrhee17

Left some minor comments but overall direction looks good 👍 Thanks @ikhoon 🙇 👍 🙇 Please merge when ready 🙇

...h2/src/main/java/com/linecorp/armeria/internal/common/auth/oauth2/AbstractOAuth2Request.java

jrhee17 · 2024-04-03T03:55:24Z

oauth2/src/main/java/com/linecorp/armeria/common/auth/oauth2/ClientAuthentication.java

+     * If null, this {@link ClientAuthentication} will be added as body parameters.
+     */
+    @Nullable
+    String asHeaderValue();


Optional) I found the logic in AbstractOAuth2Request slightly confusing.

armeria/oauth2/src/main/java/com/linecorp/armeria/internal/common/auth/oauth2/AbstractOAuth2Request.java

Lines 80 to 83 in eaaf517

final String headerValue = clientAuthentication.asHeaderValue();

if (headerValue == null) {

clientAuthentication.addAsBodyParams(formBuilder);

}

I wonder if it's more clear to create an API which looks similar to adding body parameters.

e.g.

void addHeaderValues(RequestHeadersBuilde)

By doing so, I think it is also easier to support other authentication methods easily.

Good idea. I think my thoughts were too focused on compatibility with the existing ClientAuthorization.

...h2/src/main/java/com/linecorp/armeria/client/auth/oauth2/JsonWebTokenAccessTokenRequest.java

.../src/main/java/com/linecorp/armeria/common/auth/oauth2/JsonWebTokenClientAuthentication.java

oauth2/src/main/java/com/linecorp/armeria/common/auth/oauth2/TokenRevocation.java

…/oauth2/AbstractOAuth2Request.java Co-authored-by: jrhee17 <guins_j@guins.org>

…ctor-oauth2

trustin

🚀

ikhoon added this to the 1.28.0 milestone Feb 27, 2024

ikhoon added new feature cleanup labels Feb 27, 2024

github-actions bot requested review from jrhee17, minwoox and trustin February 27, 2024 15:54

ikhoon force-pushed the refactor-oauth2 branch from e19adeb to bd84dee Compare February 27, 2024 16:04

ikhoon added 3 commits February 28, 2024 01:10

clean up

5ee2600

revert test name for easier diff

5a06093

lint

b46be49

ikhoon marked this pull request as ready for review February 28, 2024 02:42

add unit tests

58f6eab

minwoox reviewed Mar 26, 2024

View reviewed changes

Update oauth2/src/main/java/com/linecorp/armeria/client/auth/oauth2/A…

087cb30

…ccessTokenRequest.java Co-authored-by: minux <songmw725@gmail.com>

address comments by @minwoox

b9df90d

trustin reviewed Apr 1, 2024

View reviewed changes

oauth2/src/main/java/com/linecorp/armeria/common/auth/oauth2/ClientAuthentication.java Outdated Show resolved Hide resolved

...h2/src/main/java/com/linecorp/armeria/internal/common/auth/oauth2/AbstractOAuth2Request.java Outdated Show resolved Hide resolved

minwoox approved these changes Apr 2, 2024

View reviewed changes

ikhoon added 2 commits April 2, 2024 21:51

Address comments by @trustin

928bb68

clean up

eaaf517

jrhee17 approved these changes Apr 3, 2024

View reviewed changes

ikhoon and others added 4 commits April 3, 2024 15:52

Update oauth2/src/main/java/com/linecorp/armeria/internal/common/auth…

6bf43b6

…/oauth2/AbstractOAuth2Request.java Co-authored-by: jrhee17 <guins_j@guins.org>

Address comments by @jrhee17

32d917a

Merge branch 'refactor-oauth2' of github.com:ikhoon/armeria into refa…

2adbd43

…ctor-oauth2

addAsHeaders() method

267de3c

ikhoon force-pushed the refactor-oauth2 branch from f488485 to 267de3c Compare April 5, 2024 05:56

trustin approved these changes Apr 9, 2024

View reviewed changes

ikhoon added 2 commits April 9, 2024 17:52

Fix test failures

32b9e7b

missing changes

b60a9f7

ikhoon merged commit 333a16f into line:main Apr 9, 2024
16 checks passed

ikhoon deleted the refactor-oauth2 branch April 9, 2024 11:28

ikhoon mentioned this pull request Apr 25, 2024

Documentation for OAuth 2.0 client #5636

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor OAuth 2.0 modules to easily implement extension grants #5476

Refactor OAuth 2.0 modules to easily implement extension grants #5476

ikhoon commented Feb 27, 2024 •

edited

github-actions bot commented Feb 27, 2024 •

edited

ikhoon commented Feb 28, 2024

minwoox left a comment

minwoox Mar 26, 2024

ikhoon Mar 27, 2024

codecov bot commented Mar 26, 2024 •

edited

minwoox left a comment

jrhee17 left a comment •

edited

jrhee17 Apr 3, 2024

ikhoon Apr 5, 2024

trustin left a comment

	final String headerValue = clientAuthentication.asHeaderValue();
	if (headerValue == null) {
	clientAuthentication.addAsBodyParams(formBuilder);
	}

Refactor OAuth 2.0 modules to easily implement extension grants #5476

Refactor OAuth 2.0 modules to easily implement extension grants #5476

Conversation

ikhoon commented Feb 27, 2024 • edited

github-actions bot commented Feb 27, 2024 • edited

🔍 Build Scan® (commit: b60a9f7)

ikhoon commented Feb 28, 2024

minwoox left a comment

Choose a reason for hiding this comment

minwoox Mar 26, 2024

Choose a reason for hiding this comment

ikhoon Mar 27, 2024

Choose a reason for hiding this comment

codecov bot commented Mar 26, 2024 • edited

Codecov Report

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment • edited

Choose a reason for hiding this comment

jrhee17 Apr 3, 2024

Choose a reason for hiding this comment

ikhoon Apr 5, 2024

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

ikhoon commented Feb 27, 2024 •

edited

github-actions bot commented Feb 27, 2024 •

edited

🔍 Build Scan® (commit: `b60a9f7`)

codecov bot commented Mar 26, 2024 •

edited

jrhee17 left a comment •

edited