-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update KV Credential Policy #30512
Update KV Credential Policy #30512
Conversation
API change check API changes are not detected in this pull request. |
...c/main/java/com/azure/security/keyvault/secrets/implementation/KeyVaultCredentialPolicy.java
Outdated
Show resolved
Hide resolved
...n/java/com/azure/security/keyvault/certificates/implementation/KeyVaultCredentialPolicy.java
Show resolved
Hide resolved
...n/java/com/azure/security/keyvault/certificates/implementation/KeyVaultCredentialPolicy.java
Outdated
Show resolved
Hide resolved
@@ -208,6 +208,7 @@ public void authorizeRequestSync(HttpPipelineCallContext context) { | |||
.setTenantId(this.challenge.getTenantId()); | |||
|
|||
setAuthorizationHeaderSync(context, tokenRequestContext); | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This is required, it was missing before when this policy got updated via #29730
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add these changes to the KV Administration package as well.
@@ -47,6 +47,7 @@ | |||
<!-- Configures the Java 9+ run to perform the required module exports, opens, and reads that are necessary for testing but shouldn't be part of the module-info. --> | |||
<javaModulesSurefireArgLine> | |||
--add-exports com.azure.core/com.azure.core.implementation.http=ALL-UNNAMED | |||
--add-exports com.azure.core/com.azure.core.implementation.util=ALL-UNNAMED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it you needed this for running local live tests? @jairmyree and I ran into the same issue when trying to test Tables locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for running unit tests in JDK 17 locally, yeah.
This config doesn't impact users, mainly for local tests running only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a note, we also needed this for Java 11, likely for anything using Java 9+? We're considering adding this to the parent POM. /cc @srnagar
KV Administration package is autorest generated, so auto rest will take care of it when it adds sync stack to KV admin package. |
/azp run java - keyvault - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/check-enforcer override |
Live and recorded tests passed. One live test failed due to not being able to fetch a dependency (a transient issue) and another one in a special environment takes too long to execute and times out due to unrelated reasons, so we're ignoring those. |
Fixes Sync Flow of Key Vault Credential Policy