-
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
Persistence token cache for Mac & Linux with new MSAL ext #9188
Conversation
sdk/identity/azure-identity/src/main/java/com/azure/identity/DefaultAzureCredential.java
Outdated
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/DefaultAzureCredential.java
Outdated
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/DefaultAzureCredential.java
Outdated
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/DefaultAzureCredential.java
Outdated
Show resolved
Hide resolved
Thanks for the review Alan! I've addressed the comments. |
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java
Outdated
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java
Outdated
Show resolved
Hide resolved
@@ -51,7 +51,7 @@ | |||
public Mono<AccessToken> getToken(TokenRequestContext request) { | |||
return Mono.defer(() -> { | |||
if (cachedToken.get() != null) { | |||
return identityClient.authenticateWithUserRefreshToken(request, cachedToken.get()) | |||
return identityClient.authenticateWithMsalAccount(request, cachedToken.get().getAccount()) | |||
.onErrorResume(t -> Mono.empty()); |
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.
Would you want this to resume on an error and return an empty Mono? I'd expect the error to be propagated downstream and allow downstream subscribers to do with that error what they will. In globalSetupAsync
this is call is chained with .then()
, so it would hide the error.
There are similar instances in other credentials below.
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.
Yeah it should be probably be handled downstream - it's out of the scope of this PR but I can see if it can be done with minimal footprint
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java
Outdated
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java
Outdated
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java
Outdated
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java
Outdated
Show resolved
Hide resolved
@@ -51,7 +51,7 @@ public void deviceCodeCanGetToken() { | |||
Assert.assertNotNull(token.getToken()); | |||
Assert.assertNotNull(token.getExpiresAt()); | |||
Assert.assertFalse(token.isExpired()); | |||
token = client.authenticateWithUserRefreshToken(new TokenRequestContext().addScopes("https://vault.azure.net/.default"), token).block(); | |||
token = client.authenticateWithMsalAccount(new TokenRequestContext().addScopes("https://vault.azure.net/.default"), token.getAccount()).block(); |
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.
These tests could be using StepVerifier rather than block.
// Arrange
var context = new TokenRequestContext().addScopes("https://vault.azure.net/.default");
// Act & Assert
StepVerifier.create(client.authenticateWithMsalAccount(context, token.getAccount()))
.assertNext(token -> {
assertNotNull(token.getToken());
assertNotNull(token.getExpiresAt());
})
.verifyComplete();
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.
This is out of scope - if this PR stays open next week I can fix these. But for now I'm planning to get some sleep and enjoy my day off 😸
Connie's 2 feedback are out of scope of this PR. Shouldn't block this PR so I plan to fix in a separate PR. |
sdk/identity/azure-identity/src/main/java/com/azure/identity/DeviceCodeCredentialBuilder.java
Outdated
Show resolved
Hide resolved
...ity/azure-identity/src/main/java/com/azure/identity/InteractiveBrowserCredentialBuilder.java
Outdated
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/SharedTokenCacheCredential.java
Outdated
Show resolved
Hide resolved
...ntity/azure-identity/src/main/java/com/azure/identity/UsernamePasswordCredentialBuilder.java
Outdated
Show resolved
Hide resolved
Thanks for the comments. They are extremely helpful. However, some comments may be better suited as general issues for the Identity library for Java and should be considered out of scope for this PR. I'd like to narrow down the comments to only applicable for the changes in this PR. For the others, thanks @g2vinay for opening separate issues. |
Here lists the set of design decisions we need to make, to go from the MSAL shared token cache accessors to a user-friendly SharedTokenCacheCredential design that works seamlessly and effortlessly across platforms and environments.
User interface for credentials
scope
Q: What credentials are we supporting shared token cache?
A: All user account credentials and service principal credentials.
user account credentials (public client applications)
Q: Enable or disable reading from the shared token cache by default?
A: Disable.
Q: Enable or disable writing to the shared token cache by default?
A: Disable.
Q: What environment variable to specify which account if there are multiple?
A: AZURE_USERNAME.
service principal credentials (confidential client applications)
Q: Do we want to support token cache for this?
A: Yes.
Q: Enable or disable reading from the shared token cache by default?
A: Disable.
Q: Enable or disable writing from the shared token cache by default?
A: Disable.
Q: What environment variable to specify which service principal if there are multiple?
A: N/A – will be specified on the ConfidentialClientApplication
Q: If both AZURE_USERNAME and AZURE_CLIENT_ID are present, what does it do?
A: N/A
configurations
Q: Do we have one method to enable both read & write, or separate methods?
A: One method.
Q: Do we start writing to cache in SharedTokenCacheCredential now?
A: Yes
Q: Do we allow configuring where the token cache is stored?
A: No
operating system specifics
Q: Do we automatically fallback to unprotected file on Linux if Gnome keyring item is unavailable?
A: No. Allow through a property “allowUnencryptedCache”.
Shared token cache implementation
storage locations for user accounts
Q: Where to store the DPAPI encrypted file on Windows?
A: {user.home}\AppData\Local.IdentityService\msal.cache
Q: Where to store the secret item in Keychain on Mac?
A: Service = Microsoft.Developer.IdentityService, Account = MSALCache
Q: Where to store the secret item in Gnome Keyring?
A: Under default keyring, with item label “MSALCache”, and schema “msal.cache”.
Q: Where to store the secret item as an unprotected file on Linux?
A: $HOME/.IdentityService/msal.cache
(Document all these to be implementations and shouldn’t be taken as a hard dependency)
storage locations for service principals
Q: Do we merge the token cache data for service principals with the user accounts?
A: Hopefully no, to avoid service principals peeking into the user tokens
Q: If the answer is no for the above question, specify the locations for all questions in Article 2.1.
A: MSALConfidentialCache, msal.confidential.cache
Error handling & logging
exceptions
Q: What exceptions do we throw if DPAPI encrypted file cannot be found on Windows, or Keychain item is unavailable on Mac, or Gnome Keyring item is unavailable on Linux, or unprotected file is unavailable?
A: CredentialUnavailableException for SharedTokenCacheCredential, ClientAuthenticationException for others if enabled
Q: What exceptions do we throw if the file, or the Keychain/Keyring item is available, but cannot be parsed or error out for any other reasons?
A: Wrap them in ClientAuthenticationException / AuthenticationFailedException.
logging
Q: What information do we log in this process?
A: 1) What platform we are on and 2) if the token cache access succeeded or failed. No information of where the token cache is located will be logged.
transient errors
Q: Is there a plan to detect and retry transient errors, like a race condition that MSAL fails to prevent?
A: for now no.