Add instance discovery metadata to token cache#149
Conversation
| if (instanceDiscoveryResponse.metadata() != null) { | ||
| for (InstanceDiscoveryMetadataEntry entry : instanceDiscoveryResponse.metadata()) { | ||
|
|
||
| // expiry time of 96 hours |
There was a problem hiding this comment.
What should expiry date be?
| sendInstanceDiscoveryRequest(authorityUrl, msalRequest, serviceBundle); | ||
|
|
||
| if (validateAuthority) { | ||
| validate(instanceDiscoveryResponse); |
There was a problem hiding this comment.
[Aside] The way you do validate is different from how .net does it so it might be a good idea to align here... Can you give me an example where the instance discovery endpoint would fail validation ? I manually tested using a "bogus authority" and the instance discovery endpoint fails with a 500 error I believe.
There was a problem hiding this comment.
We are not validating instance discovery endpoint here, only validating that the response contains "tenant_discovery_endpoint". Authority validation happens here: https://github.com/AzureAD/microsoft-authentication-library-for-java/blob/dev/src/main/java/com/microsoft/aad/msal4j/Authority.java#L88
What validation happens in .net? Agreed that we should align here.
| protected static final int MIN_ACCESS_TOKEN_EXPIRE_IN_SEC = 5 * 60; | ||
|
|
||
| transient private ReadWriteLock lock = new ReentrantReadWriteLock(); | ||
| transient ReadWriteLock lock = new ReentrantReadWriteLock(); |
There was a problem hiding this comment.
Good point, will fix.
|
|
||
| URL authority = new URL(app.authority()); | ||
|
|
||
| PowerMock.mockStaticPartial(AadInstanceDiscovery.class, "sendInstanceDiscoveryRequest"); |
There was a problem hiding this comment.
[Aside] If you use an interface and either make your service a singleton or just pass the instance around, the code would be easier to test without tools for "untestable code"
| "client_id": "my_client_id" | ||
| } | ||
| }, | ||
| "InstanceDiscoveryMetadata": { |
There was a problem hiding this comment.
MSAL .net should be ok with this, it is similar to the "unknown entity" above. Otherwise, the cache structure looks good to me, but please open an API review so that this is documented for other teams / new MSALs to come.
There was a problem hiding this comment.
+1 : to get this documented
bgavrilMS
left a comment
bgavrilMS
left a comment
There was a problem hiding this comment.
Generally looks good, please document the cache changes and consider refactoring the instance metadata service to be non-static.
4 participants
In the case where a client application object is create in a new process, and they are using a token cache that has been previously written to disk, then they can use the instance discovery metadata from the token cache instead of having to make a network call.