Skip to content

Support TenantId and AuthorityHost in APPLICATIONINSIGHTS_AUTHENTICATION_STRING - #4892

Open
Rajkumar Rangaraj (rajkumar-rangaraj) wants to merge 2 commits into
mainfrom
fix/aad-auth-string-tenant-id
Open

Rajkumar Rangaraj (rajkumar-rangaraj) wants to merge 2 commits into
mainfrom
fix/aad-auth-string-tenant-id

Conversation

@rajkumar-rangaraj

Copy link
Copy Markdown
Member

Fix #4891 .

APPLICATIONINSIGHTS_AUTHENTICATION_STRING=Authorization=AAD;ClientId=...;ClientSecret=... (the replacement recommended by the clientSecret JSON deprecation warning) could never work: overlayAadEnvVars replaces the whole authentication section and never sets tenantId, so AadAuthentication.validate() always failed with missing "tenantId" (and ClientSecretCredentialBuilder requires it anyway).

Changes:

  • Read TenantId and optional AuthorityHost from APPLICATIONINSIGHTS_AUTHENTICATION_STRING when ClientSecret is present, e.g. Authorization=AAD;ClientId={CLIENT_ID};ClientSecret={CLIENT_SECRET};TenantId={TENANT_ID}
  • Update the clientSecret deprecation warning example and the missing-tenantId error message to mention TenantId
  • Extend shouldOverrideAadAuthenticationConfig to cover TenantId/AuthorityHost and to call validate() (the previous test didn't, which is why this wasn't caught)

Validation:

  • ./gradlew :agent:agent-tooling:spotlessApply :agent:agent-tooling:test --tests "*ConfigurationTest": passed (46 tests, 0 failures)

For significant contributions please make sure you have completed the following items:

…ION_STRING

Client secret authentication configured via APPLICATIONINSIGHTS_AUTHENTICATION_STRING
always failed validation because there was no way to provide the tenant id.

Fixes #4891

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The tenant ID error guidance should direct users of APPLICATIONINSIGHTS_AUTHENTICATION_STRING to update that variable, since it overrides the JSON authentication section and makes the suggested JSON fix ineffective.

1 open issue
What changed in this PR

Fixes #4891 by allowing the Java agent’s environment-based client-secret authentication to supply the required tenant ID and an optional authority host.

Changes:

  • Parse TenantId and AuthorityHost from APPLICATIONINSIGHTS_AUTHENTICATION_STRING.
  • Update authentication warnings, validation guidance, and configuration comments.
  • Extend regression tests to validate authentication configurations and document the fix.
File Description
CHANGELOG.md Records the environment-based authentication fix.
agent/​agent-tooling/​src/​test/​java/​com/​microsoft/​applicationinsights/​agent/​internal/​configuration/​ConfigurationTest.java Covers missing tenant IDs and valid configurations with optional authority hosts.
agent/​agent-tooling/​src/​main/​java/​com/​microsoft/​applicationinsights/​agent/​internal/​configuration/​ConfigurationBuilder.java Reads the new fields and updates the deprecation warning example.
agent/​agent-tooling/​src/​main/​java/​com/​microsoft/​applicationinsights/​agent/​internal/​configuration/​Configuration.java Updates missing-tenant guidance and the authentication example comment.

Review effort: Max

Comment on lines +1622 to +1623
"Please provide a valid \"tenantId\" under the \"authentication\" configuration, "
+ "or add \"TenantId={TENANT_ID}\" to the APPLICATIONINSIGHTS_AUTHENTICATION_STRING environment variable. "
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.

APPLICATIONINSIGHTS_AUTHENTICATION_STRING with ClientSecret cannot work – no way to provide tenantId

2 participants