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

feat(java-sdk): configurable token endpoint #240

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

le-yams
Copy link
Contributor

@le-yams le-yams commented Nov 30, 2023

Configurable client_credentials token endpoint for the Java SDK.

Description

Add a tokenEndpoint property to the ClientCredentials class. The OAuth2Client use this property value if not null/blank to query the token. If the property is not set, the previous behavior is applied (default to <issuer>/oauth/token).

References

#238

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

@le-yams le-yams requested a review from a team as a code owner November 30, 2023 08:17
Copy link

linux-foundation-easycla bot commented Nov 30, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@le-yams le-yams force-pushed the feat/java_configurable_token_endpoint branch from 293fdd7 to 555a773 Compare November 30, 2023 08:35
@le-yams le-yams changed the title feat: java configurable token endpoint feat(java-sdk): configurable token endpoint Nov 30, 2023
@booniepepper
Copy link
Contributor

booniepepper commented Dec 1, 2023

Thanks for taking the time to put this together. Can you help me understand the problem you are trying to solve?

Do you need to customize the protocol/scheme (currently hardcoded as https) or to customize the path (currently hardcoded as /oauth/token)? Is it something different entirely?

If possible, I'd rather allow those parts to be configured independently rather than override everything (including/duplicating the issuer)

@le-yams
Copy link
Contributor Author

le-yams commented Dec 1, 2023

Thanks for taking the time to put this together. Can you help me understand the problem you are trying to solve?

Do you need to customize the protocol/scheme (currently hardcoded as https) or to customize the path (currently hardcoded as /oauth/token)? Is it something different entirely?

Sorry I wasn't clear enough. The problem I'd like to solve is : using a different token endpoint than the currently hardcoded as /oauth/token.
I can think of 2 solutions (not exclusives) :

  • make the endpoint url configurable/overridable
  • use oidc discovery

The second solution is less trivial to implement and only work if the authority server support oidc so the PR is about the first one.

If possible, I'd rather allow those parts to be configured independently rather than override everything (including/duplicating the issuer)

That's a good point and I indeed asked myself if I should make only the path configurable rather than the full url.
I choose the former because all tools/UIs I have used for such configuration make you specify the full url (you may rightfully argue that this is some kind of cargo cult on me ^^).
Also, when doing a client_credentials flow, the concept of "issuer" doesn't really exist there's only the authority token endpoint (the issuer concept is used on the server side for validating the token). That make me think that there also is no "audience" concept from the client point of view but only optional scopes (but that would be another issue/PR).

So maybe the proper way would be to replace the "issuer" input with a "tokenEndpoint" one (so the sdk user directly specify the full token and thus make the implicit explicit).
That would be a breaking change, that's why I kept the two inputs in this PR. Maybe the issuer could be made optional, the validation would check that either the issuer either the token endpoint is specified (specifying the issuer would use the harcoded path). I would also document the issuer input as deprecated.

That's just my thought and I look forward to yours :)

Thanks for taking the time into looking at this PR 😄

@le-yams le-yams force-pushed the feat/java_configurable_token_endpoint branch from 555a773 to 234e1cd Compare December 12, 2023 16:43
@rhamzeh
Copy link
Member

rhamzeh commented Dec 12, 2023

Hi @le-yams - sorry about taking time to respond.

Just wanted to first say thanks a lot for working on this ❤️

We'll gladly accept the contribution, but may I ask for some changes?

Instead of introducing a new field, we'd rather change the apiTokenIssuer endpoint to accept a full url.
So:

ApiTokenIssuer Endpoint SDK will hit
issuer.fga.example https://issuer.fga.example/oauth/token
https://issuer.fga.example https://issuer.fga.example/oauth/token
https://issuer.fga.example:8080 https://issuer.fga.example:8080/oauth/token
issuer.fga.example/some_endpoint https://issuer.fga.example/some_endpoint
https://issuer.fga.example/some_endpoint https://issuer.fga.example/some_endpoint
https://issuer.fga.example:8080/some_endpoint https://issuer.fga.example:8080/some_endpoint

Of course, we'll need to do some of the validations to ensure e.g. people are passing fields with https or http

See for example the change we introduced in the last release of the Go SDK: openfga/go-sdk@cdd7206

@le-yams le-yams force-pushed the feat/java_configurable_token_endpoint branch from 234e1cd to d973ac2 Compare December 13, 2023 08:24
@le-yams
Copy link
Contributor Author

le-yams commented Dec 13, 2023

Hi @rhamzeh , I updated the PR according to your comment. I made the test cover all the cases you mentioned (plus some with the authority having a final /).

Copy link
Member

@rhamzeh rhamzeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @le-yams!

There's some minor things to be done:

  • renaming tokenEndpointUrl to apiTokenIssuer
  • validating that the scheme is http/https
  • adding tests for invalid schemes/uris

But we'll do that in a separate PR after merging.

Expect this PR to go out in the next v0.3.0 release, to be out soon

@rhamzeh rhamzeh added this pull request to the merge queue Dec 13, 2023
Merged via the queue into openfga:main with commit a86538c Dec 13, 2023
2 of 7 checks passed
@le-yams le-yams deleted the feat/java_configurable_token_endpoint branch December 14, 2023 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants