Add ClientInstanceTokenBinding type #2304

VivekVinushanth · 2023-12-14T04:09:16Z

Proposed changes in this pull request

This PR adds new binding type where the client instance can send an identifier of its preference and request the IS to bind itself to the token. This provides the capability for grant types like password or token exchange where session/cookie based binding types cannot be used and also ensures each client instance gets token so that eventually the client has multiple active access token.

Fixes: wso2/product-is#18600

When should this PR be merged

[Please describe any preconditions that need to be addressed before we
can merge this pull request.]

Follow up actions

[List any possible follow-up actions here; for instance, testing data
migrations, software that we need to install on staging and production
environments.]

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly?

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
Are all UI and API inputs run through forms or serializers?
Are all external inputs validated and sanitized appropriately?
Does all branching logic have a default case?
Does this solution handle outliers and edge cases gracefully?
Are all external communications secured and restricted to SSL?

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes should be documented.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

…anceBindingType

senthalan · 2023-12-14T04:42:24Z

...on.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/AccessTokenIssuer.java

@@ -1065,11 +1066,23 @@ private void handleTokenBinding(OAuth2AccessTokenReqDTO tokenReqDTO, String gran
                throw new IdentityOAuth2ClientException(OAuth2ErrorCodes.INVALID_REQUEST,
                        "TLS certificate not found in the request.");
            }
+            if (OAuth2Constants.TokenBinderType.CLIENT_INSTANCE.equals(tokenBinder.getBindingType())) {


Having specific checks for implementation in the core is not a good practice. What is the reason for these checks?

If the client instance has not sent any value, we treat as no binding type (as the binding is set to null by default)

senthalan · 2023-12-14T04:43:06Z

...on.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/AccessTokenIssuer.java

            throw new IdentityOAuth2Exception(
                    "Token binding reference cannot be retrieved form the token binder: " + tokenBinder
                            .getBindingType());
        }

+        if (OAuth2Constants.TokenBinderType.CLIENT_INSTANCE.equals(tokenBinder.getBindingType()) &&
+                tokenBindingValueOptional.get().length() >= MAX_ALLOWED_LENGTH) {


can't we do this validation within the ClientInstanceTokenBinder itself ?

We can but we are unable to throw a client exception from the binder itself.

senthalan · 2023-12-14T04:45:15Z

...main/java/org/wso2/carbon/identity/oauth2/token/bindings/impl/ClientInstanceTokenBinder.java

+
+        RequestParameter[] parameters = oAuth2AccessTokenReqDTO.getRequestParameters();
+        for (RequestParameter parameter : parameters) {
+            if (CLIENT_INSTANCE_REF.equals(parameter.getKey())


can we verify how easy is to send this parameter in the token request using the SDk ? does SDKs allow to add extra parameter in the token request ?

AFAIK, our SDKs do not support sending additional parameters. But wondering back-channel grant types like password/token-exchange require an SDK?

jenkins-is-staging · 2023-12-14T06:23:49Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/7205280151

jenkins-is-staging · 2023-12-14T07:59:46Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/7205280151
Status: failure

jenkins-is-staging · 2023-12-15T03:29:52Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/7205280151

jenkins-is-staging · 2023-12-15T05:00:32Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/7205280151
Status: failure

jenkins-is-staging · 2023-12-15T11:50:33Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/7205280151

jenkins-is-staging · 2023-12-15T13:23:21Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/7205280151
Status: success

jenkins-is-staging

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/7205280151

kayathiri4 · 2024-01-02T06:00:43Z

.../main/java/org/wso2/carbon/identity/oauth2/token/bindings/impl/ClientRequestTokenBinder.java

@@ -0,0 +1,108 @@
+/*
+ * Copyright (c) 2022, WSO2 Inc. (http://www.wso2.com).


Shall we update License header

VivekVinushanth added 3 commits December 14, 2023 09:28

Add ClientInstanceTokenBinding

5d916f0

Merge remote-tracking branch 'upstream/master' into vv-add-clientInst…

a53a7a2

…anceBindingType

Fix checkstyle

8cb2bde

senthalan reviewed Dec 14, 2023

View reviewed changes

Update error message

4980141

VivekVinushanth requested a review from senthalan December 15, 2023 06:02

change bindingType

1990572

change bindingType

23eb69a

jenkins-is-staging approved these changes Dec 15, 2023

View reviewed changes

senthalan approved these changes Dec 18, 2023

View reviewed changes

VivekVinushanth merged commit e386e31 into wso2-extensions:master Dec 18, 2023
1 check passed

kayathiri4 reviewed Jan 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ClientInstanceTokenBinding type #2304

Add ClientInstanceTokenBinding type #2304

VivekVinushanth commented Dec 14, 2023 •

edited

Loading

senthalan Dec 14, 2023

VivekVinushanth Dec 14, 2023

senthalan Dec 14, 2023

VivekVinushanth Dec 14, 2023

senthalan Dec 14, 2023

VivekVinushanth Dec 14, 2023

jenkins-is-staging commented Dec 14, 2023

jenkins-is-staging commented Dec 14, 2023

jenkins-is-staging commented Dec 15, 2023

jenkins-is-staging commented Dec 15, 2023

jenkins-is-staging commented Dec 15, 2023

jenkins-is-staging commented Dec 15, 2023

jenkins-is-staging left a comment

kayathiri4 Jan 2, 2024

		@@ -0,0 +1,108 @@
		/*
		* Copyright (c) 2022, WSO2 Inc. (http://www.wso2.com).

Add ClientInstanceTokenBinding type #2304

Add ClientInstanceTokenBinding type #2304

Conversation

VivekVinushanth commented Dec 14, 2023 • edited Loading

Proposed changes in this pull request

When should this PR be merged

Follow up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

senthalan Dec 14, 2023

Choose a reason for hiding this comment

VivekVinushanth Dec 14, 2023

Choose a reason for hiding this comment

senthalan Dec 14, 2023

Choose a reason for hiding this comment

VivekVinushanth Dec 14, 2023

Choose a reason for hiding this comment

senthalan Dec 14, 2023

Choose a reason for hiding this comment

VivekVinushanth Dec 14, 2023

Choose a reason for hiding this comment

jenkins-is-staging commented Dec 14, 2023

jenkins-is-staging commented Dec 14, 2023

jenkins-is-staging commented Dec 15, 2023

jenkins-is-staging commented Dec 15, 2023

jenkins-is-staging commented Dec 15, 2023

jenkins-is-staging commented Dec 15, 2023

jenkins-is-staging left a comment

Choose a reason for hiding this comment

kayathiri4 Jan 2, 2024

Choose a reason for hiding this comment

VivekVinushanth commented Dec 14, 2023 •

edited

Loading