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.

Sign up for GitHub

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

Jump to bottom

Make dcr changes to add tls bound access token as a token binding type #2235

Merged
merged 15 commits into from
Nov 13, 2023
Merged

Make dcr changes to add tls bound access token as a token binding type #2235

merged 15 commits into from
Nov 13, 2023

Conversation

SachiniSiriwardene
Copy link
Contributor

Proposed changes in this pull request

Map the tls_client_certificate_bound_access_tokens property as a token binding type in the DCR flow.

[List all changes you want to add here. If you fixed an issue, please
add a reference to that issue as well.]

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.

@SachiniSiriwardene
make dcr changes to add tls bound access token as a token binding type
a533c97
@SachiniSiriwardene
remove empty checks for update request
cb6c950
@SachiniSiriwardene
Merge branch 'master' of https://github.com/wso2-extensions/identity-…
d441f2c 
…inbound-auth-oauth into master_DCR_Validations

� Conflicts:
�	components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java
�	components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthUtil.java
@SachiniSiriwardene
remove empty checks for update request
7267f6d
@SachiniSiriwardene
Merge branch 'master' of https://github.com/wso2-extensions/identity-…
1194345 
…inbound-auth-oauth into master_DCR_Validations
ChinthakaJ98
ChinthakaJ98 reviewed Nov 9, 2023
View reviewed changes
...dentity.oauth.common/src/main/java/org/wso2/carbon/identity/oauth/common/OAuthConstants.java Outdated
@@ -247,6 +247,9 @@ public static SubjectType fromValue(String text) {
"SupportedIDTokenEncryptionMethod";
public static final String REQUEST_OBJECT_ENCRYPTION_METHOD = "OAuth.OpenIDConnect." +
"SupportedRequestObjectEncryptionMethods.SupportedRequestObjectEncryptionMethod";

public static final String ENABLE_TLS_CERT_TOKEN_BINDING = "OAuth.OpenIDConnect." +
Copy link
Contributor

Choose a reason for hiding this comment

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

We have already added a constant for this ENABLE_TLS_CERT_BOUND_ACCESS_TOKENS_VIA_BINDING_TYPE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

...dentity.oauth.common/src/main/java/org/wso2/carbon/identity/oauth/common/OAuthConstants.java Outdated
@@ -642,6 +645,8 @@ private SignatureAlgorithms() {
public static class TokenBindings {

public static final String NONE = "NONE";

public static final String CERTIFICATE_BASED_TOKEN_BINDER = "certificate";
Copy link
Contributor

Choose a reason for hiding this comment

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

We do have a constant for this as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java Outdated
@@ -100,8 +100,7 @@
import static org.wso2.carbon.identity.oauth.Error.INVALID_REQUEST;
import static org.wso2.carbon.identity.oauth.OAuthUtil.handleError;
import static org.wso2.carbon.identity.oauth.OAuthUtil.handleErrorWithExceptionType;
import static org.wso2.carbon.identity.oauth.common.OAuthConstants.OauthAppStates.APP_STATE_ACTIVE;
import static org.wso2.carbon.identity.oauth.common.OAuthConstants.OauthAppStates.APP_STATE_DELETED;
import static org.wso2.carbon.identity.oauth.common.OAuthConstants.OauthAppStates.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we add the static imports separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

janakamarasena
janakamarasena reviewed Nov 10, 2023
View reviewed changes
...identity.oauth.dcr/src/main/java/org/wso2/carbon/identity/oauth/dcr/service/DCRMService.java Outdated
Comment on lines 332 to 333
if (Boolean.parseBoolean(IdentityUtil.getProperty(
OAuthConstants.ENABLE_TLS_CERT_BOUND_ACCESS_TOKENS_VIA_BINDING_TYPE))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of checking for the config here lets check if the CERTIFICATE_BASED_TOKEN_BINDER component is available and then set it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

janakamarasena
janakamarasena reviewed Nov 10, 2023
View reviewed changes
...identity.oauth.dcr/src/main/java/org/wso2/carbon/identity/oauth/dcr/service/DCRMService.java Outdated
Comment on lines 636 to 637
if (Boolean.parseBoolean(IdentityUtil.getProperty(
OAuthConstants.ENABLE_TLS_CERT_BOUND_ACCESS_TOKENS_VIA_BINDING_TYPE))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of checking for the config here lets check if the CERTIFICATE_BASED_TOKEN_BINDER component is available and then set it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

janakamarasena
janakamarasena previously approved these changes Nov 10, 2023
View reviewed changes
@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

janakamarasena
janakamarasena previously approved these changes Nov 13, 2023
View reviewed changes
@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

jenkins-is-staging
jenkins-is-staging approved these changes Nov 13, 2023
View reviewed changes
Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

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

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

@janakamarasena janakamarasena merged commit 2750cdc into wso2-extensions:master Nov 13, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChinthakaJ98 ChinthakaJ98 ChinthakaJ98 left review comments

@jenkins-is-staging jenkins-is-staging jenkins-is-staging approved these changes

@janakamarasena janakamarasena janakamarasena approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SachiniSiriwardene @jenkins-is-staging @janakamarasena @ChinthakaJ98