Skip to content

Conversation

@rujche
Copy link
Member

@rujche rujche commented Jun 19, 2024

Description

Fix #40616

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@rujche rujche self-assigned this Jun 19, 2024
@rujche rujche added azure-spring All azure-spring related issues azure-spring-jca labels Jun 19, 2024
@rujche rujche added this to the 2024-07 milestone Jun 19, 2024
.append(RESOURCE_FRAGMENT).append(resource);
.append(CLIENT_ID_FRAGMENT).append(URLEncoder.encode(clientId, StandardCharsets.UTF_8))
.append(CLIENT_SECRET_FRAGMENT).append(URLEncoder.encode(clientSecret, StandardCharsets.UTF_8))
.append(RESOURCE_FRAGMENT).append(URLEncoder.encode(resource, StandardCharsets.UTF_8));
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI:

  1. According to the ms doc, every parameters should url encoded, not encode one specific parameter nor the whole requestBody.
  2. According to the ms doc, the parameter should be scope, not resource. I don't know why it use resource here. Anyway, I just keep the original behavior.

Refs: https://learn.microsoft.com/en-us/entra/identity-platform/v2-oauth2-client-creds-grant-flow#first-case-access-token-request-with-a-shared-secret

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@rujche
Copy link
Member Author

rujche commented Jun 20, 2024

Hi, @saragluna , please help to review this PR.

Copy link
Member

@saragluna saragluna left a comment

Choose a reason for hiding this comment

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

Is this change tested?

@rujche
Copy link
Member Author

rujche commented Jun 20, 2024

Is this change tested?

@saragluna
Tested in my localhost.
Seems java -keyvault - ci skipped this test. I'll try to use java - keyvault - tests to test it.

@rujche
Copy link
Member Author

rujche commented Jun 20, 2024

/azp run java - keyvault - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rujche
Copy link
Member Author

rujche commented Jun 20, 2024

/azp run java - keyvault - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rujche rujche merged commit eb0d42f into Azure:main Jun 20, 2024
@rujche rujche deleted the Fix-AccessTokenUtil-does-not-urlencode-its-parameters branch June 20, 2024 06:22
Netyyyy pushed a commit to Netyyyy/azure-sdk-for-java that referenced this pull request Jul 11, 2024
* Fix bug: clientSecret not url encoded.

(cherry picked from commit eb0d42f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

azure-spring All azure-spring related issues azure-spring-jca

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Keyvault JCA's AccessTokenUtil does not urlencode its parameters when getting an access token

3 participants