Skip to content

Commit

Permalink
Avoid exposing identityClientOptions (#37546)
Browse files Browse the repository at this point in the history
* Avoid exposing identityClientOptions

Instead of making this field protected we'll access it with reflection from the new broker builder.

* Use a helper type instead

* Cleanup loggers

* unused import
  • Loading branch information
billwert committed Nov 8, 2023
1 parent 8410cc7 commit 258e7c6
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 6 deletions.
Expand Up @@ -18,6 +18,7 @@
import com.azure.identity.InteractiveBrowserCredential;
import com.azure.identity.InteractiveBrowserCredentialBuilder;
import com.azure.identity.TokenCachePersistenceOptions;
import com.azure.identity.implementation.CredentialBuilderBaseHelper;

import java.time.Duration;
import java.util.List;
Expand All @@ -38,15 +39,14 @@
* <!-- end com.azure.identity.broker.interactivebrowserbrokercredentialbuilder.construct -->
*/
public class InteractiveBrowserBrokerCredentialBuilder extends InteractiveBrowserCredentialBuilder {

/**
* Sets the parent window handle used by the broker. For use on Windows only.
*
* @param windowHandle The window handle of the current application, or 0 for a console application.
* @return An updated instance of this builder with the interactive browser broker configured.
*/
public InteractiveBrowserBrokerCredentialBuilder setWindowHandle(long windowHandle) {
this.identityClientOptions.setBrokerWindowHandle(windowHandle);
CredentialBuilderBaseHelper.getClientOptions(this).setBrokerWindowHandle(windowHandle);
return this;
}

Expand All @@ -57,7 +57,7 @@ public InteractiveBrowserBrokerCredentialBuilder setWindowHandle(long windowHand
* @return An updated instance of this builder with enable Legacy MSA Passthrough set to true.
*/
public InteractiveBrowserBrokerCredentialBuilder enableLegacyMsaPassthrough() {
this.identityClientOptions.setEnableLegacyMsaPassthrough(true);
CredentialBuilderBaseHelper.getClientOptions(this).setEnableLegacyMsaPassthrough(true);
return this;
}

Expand Down
Expand Up @@ -17,6 +17,7 @@
import com.azure.core.util.Configuration;
import com.azure.core.util.HttpClientOptions;
import com.azure.core.util.logging.ClientLogger;
import com.azure.identity.implementation.CredentialBuilderBaseHelper;
import com.azure.identity.implementation.IdentityClientOptions;

import java.time.Duration;
Expand All @@ -29,11 +30,18 @@
*/
public abstract class CredentialBuilderBase<T extends CredentialBuilderBase<T>> implements HttpTrait<T> {
private static final ClientLogger LOGGER = new ClientLogger(CredentialBuilderBase.class);

static {
CredentialBuilderBaseHelper.setAccessor(new CredentialBuilderBaseHelper.CredentialBuilderBaseAccessor() {
@Override
public IdentityClientOptions getClientOptions(CredentialBuilderBase<?> builder) {
return builder.identityClientOptions;
}
});
}
/**
* The options for configuring the identity client.
*/
protected IdentityClientOptions identityClientOptions;
IdentityClientOptions identityClientOptions;

CredentialBuilderBase() {
this.identityClientOptions = new IdentityClientOptions();
Expand Down
@@ -0,0 +1,39 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.identity.implementation;

import com.azure.core.util.logging.ClientLogger;
import com.azure.identity.CredentialBuilderBase;

/**
* Helper type for accessing private fields of {@link CredentialBuilderBase}.
*/
public final class CredentialBuilderBaseHelper {
private static final ClientLogger LOGGER = new ClientLogger(CredentialBuilderBaseHelper.class);
private static CredentialBuilderBaseAccessor accessor;

private CredentialBuilderBaseHelper() { }

public interface CredentialBuilderBaseAccessor {
IdentityClientOptions getClientOptions(CredentialBuilderBase<?> builder);
}

public static void setAccessor(final CredentialBuilderBaseAccessor newAccessor) {
if (accessor != null) {
throw LOGGER.logExceptionAsError(new NullPointerException("Accessor must be non-null"));

This comment has been minimized.

Copy link
@NsrM

NsrM Nov 10, 2023

Hey, accessor is already a non-null in your if condition. I feel "Accessor must be non-null" being thrown in Logger is not correct.
Instead I wonder if you actually wanted to write Acessor must be null.
Please recheck this code block. Its contradicting itself is what I feel. Thank you

}
accessor = newAccessor;
}

public static CredentialBuilderBaseAccessor getAccessor() {
if (accessor == null) {
throw LOGGER.logExceptionAsError(new IllegalStateException("CredentialBuilderBaseHelper must be initialized"));
}
return accessor;
}

public static IdentityClientOptions getClientOptions(CredentialBuilderBase<?> builder) {
return getAccessor().getClientOptions(builder);
}
}
2 changes: 1 addition & 1 deletion sdk/identity/azure-identity/src/main/java/module-info.java
Expand Up @@ -13,6 +13,6 @@

exports com.azure.identity;
exports com.azure.identity.implementation to com.azure.identity.broker;

opens com.azure.identity to com.azure.identity.broker;
opens com.azure.identity.implementation to com.fasterxml.jackson.databind, com.azure.identity.broker, com.azure.core;
}

0 comments on commit 258e7c6

Please sign in to comment.