Skip to content
This repository has been archived by the owner on Jun 30, 2023. It is now read-only.

fix null ref in tokenCache when UserInfo is absent #1607

Merged
merged 2 commits into from
Jun 3, 2019
Merged

fix null ref in tokenCache when UserInfo is absent #1607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f3c39f
fix null ref in tokenCache when UserInfo is absent
jennyf19 May 30, 2019
49f8e6c
pr comments to fix utest
jennyf19 Jun 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 23 additions & 23 deletions src/Microsoft.IdentityModel.Clients.ActiveDirectory/Core/Cache/AdalTokenCacheKey.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,18 +57,18 @@ internal enum TokenSubjectType
internal sealed class AdalTokenCacheKey
{
internal AdalTokenCacheKey(string authority, string resource, string clientId, TokenSubjectType tokenSubjectType, AdalUserInfo adalUserInfo)
: this(authority, resource, clientId, tokenSubjectType, (adalUserInfo != null) ? adalUserInfo.UniqueId : null, (adalUserInfo != null) ? adalUserInfo.DisplayableId : null)
: this(authority, resource, clientId, tokenSubjectType, adalUserInfo?.UniqueId, adalUserInfo?.DisplayableId)
{
}

internal AdalTokenCacheKey(string authority, string resource, string clientId, TokenSubjectType tokenSubjectType, string uniqueId, string displayableId)
{
this.Authority = authority;
this.Resource = resource;
this.ClientId = clientId;
this.TokenSubjectType = tokenSubjectType;
this.UniqueId = uniqueId;
this.DisplayableId = displayableId;
Authority = authority;
Resource = resource;
ClientId = clientId;
TokenSubjectType = tokenSubjectType;
UniqueId = uniqueId;
DisplayableId = displayableId;
}

public string Authority { get; }
Expand All @@ -93,7 +93,7 @@ internal AdalTokenCacheKey(string authority, string resource, string clientId, T
public override bool Equals(object obj)
{
AdalTokenCacheKey other = obj as AdalTokenCacheKey;
return (other != null) && this.Equals(other);
return (other != null) && Equals(other);
}

/// <summary>
Expand All @@ -111,12 +111,12 @@ public bool Equals(AdalTokenCacheKey other)
}

return other != null
&& other.Authority == this.Authority
&& this.ResourceEquals(other.Resource)
&& this.ClientIdEquals(other.ClientId)
&& other.UniqueId == this.UniqueId
&& this.DisplayableIdEquals(other.DisplayableId)
&& other.TokenSubjectType == this.TokenSubjectType;
&& other.Authority == Authority
&& ResourceEquals(other.Resource)
&& ClientIdEquals(other.ClientId)
&& other.UniqueId == UniqueId
&& DisplayableIdEquals(other.DisplayableId)
&& other.TokenSubjectType == TokenSubjectType;
}

/// <summary>
Expand All @@ -128,28 +128,28 @@ public bool Equals(AdalTokenCacheKey other)
public override int GetHashCode()
{
const string delimiter = ":::";
var hashString = this.Authority + delimiter
+ this.Resource.ToLowerInvariant() + delimiter
+ this.ClientId.ToLowerInvariant() + delimiter
+ this.UniqueId + delimiter
+ this.DisplayableId?.ToLowerInvariant() + delimiter
+ (int) this.TokenSubjectType;
var hashString = Authority + delimiter
+ Resource.ToLowerInvariant() + delimiter
+ ClientId.ToLowerInvariant() + delimiter
+ UniqueId + delimiter
+ DisplayableId?.ToLowerInvariant() + delimiter
+ (int) TokenSubjectType;
return hashString.GetHashCode();
}

internal bool ResourceEquals(string otherResource)
{
return (string.Compare(otherResource, this.Resource, StringComparison.OrdinalIgnoreCase) == 0);
return (string.Compare(otherResource, Resource, StringComparison.OrdinalIgnoreCase) == 0);
}

internal bool ClientIdEquals(string otherClientId)
{
return (string.Compare(otherClientId, this.ClientId, StringComparison.OrdinalIgnoreCase) == 0);
return (string.Compare(otherClientId, ClientId, StringComparison.OrdinalIgnoreCase) == 0);
}

internal bool DisplayableIdEquals(string otherDisplayableId)
{
return (string.Compare(otherDisplayableId, this.DisplayableId, StringComparison.OrdinalIgnoreCase) == 0);
return (string.Compare(otherDisplayableId, DisplayableId, StringComparison.OrdinalIgnoreCase) == 0);
}

private string DebuggerDisplay
Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.IdentityModel.Clients.ActiveDirectory/TokenCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,8 +690,8 @@ internal void StoreToCacheCommon(AdalResultWrapper result, string authority, str
IdToken.TryParse(result.Result.IdToken, out IdToken idToken);

CacheFallbackOperations.WriteMsalRefreshToken(TokenCacheAccessor, result, authority, clientId, displayableId,
result.Result.UserInfo.GivenName,
result.Result.UserInfo.FamilyName,
result.Result.UserInfo?.GivenName,
result.Result.UserInfo?.FamilyName,
Copy link
Contributor Author

@jennyf19 jennyf19 May 30, 2019 •
edited by henrik-me
Loading

Choose a reason for hiding this comment

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

adding ? is the actual fix. the customer was getting a null ref because UserInfo is null here. There will no longer be a null ref here.
image
#Closed

Copy link
Contributor

@henrik-me henrik-me May 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Is it then possible to complete the scenario? I believe the MSAL cache will now be empty and thus only when using the ADAL cache will there be any values in the cache... thus depending on how the customer persists the cache (which serializer/deserializer) this may or may not work? #Closed

Copy link
Contributor Author

@jennyf19 jennyf19 May 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

you are right. if there is no id_token, then we will exit out of writing items to the MSAL cache...see here however, i'm assuming, since this has been out for awhile, that it hasn't been very impactful, or our customers are not using the cache in that way?

In reply to: 288842460 [](ancestors = 288842460)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed the comments. lmk if you'd like to discuss anything further. thanks for the utest suggestions and improvements.

In reply to: 288855740 [](ancestors = 288855740,288842460)

idToken?.GetUniqueId());
}
}
Expand Down
90 changes: 88 additions & 2 deletions tests/Test.ADAL.NET.Unit.net45/TokenCacheUnitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
using Microsoft.Identity.Core;
using Microsoft.Identity.Core.Cache;
using Microsoft.IdentityModel.Clients.ActiveDirectory.Internal.Cache;
using Test.ADAL.NET.Common.Mocks;
using System;

namespace Test.ADAL.NET.Unit
{
Expand Down Expand Up @@ -186,11 +188,95 @@ public void ParallelStoreTest()
}

[TestMethod]
[Description("Test to ensure the token cache doesnt throw an exception when cleared")]
[Description("Test to ensure the token cache doesn't throw an exception when cleared")]
[TestCategory("AdalDotNet.Unit")]
public void TokenCacheClearTest()
{
TokenCacheTests.TokenCacheClearTest(File.ReadAllBytes("oldcache.serialized"));
}

[TestMethod]
[Description("Test to ensure a null IdToken is handled correctly")]
[TestCategory("AdalDotNet.Unit")]
public void NullIdTokenCacheTest()
{
// arrange & act
TokenCache tokenCache = new TokenCache();

var result = CreateAdalResultWithIdToken(false);

WriteMsalRefreshTokenValues(result, tokenCache.TokenCacheAccessor);

// assert
// no IdToken present, CacheFallbackOperations exits early
Assert.AreEqual(tokenCache.TokenCacheAccessor.AccountCount, 0);
Assert.AreEqual(tokenCache.TokenCacheAccessor.RefreshTokenCount, 0);
Assert.IsNull(result.Result.UserInfo);
}

[TestMethod]
[Description("Test to ensure WriteMsalRefreshToken handles presence of IdToken correctly")]
[TestCategory("AdalDotNet.Unit")]
public void IdTokenReturnedCacheTest()
{
// arrange
TokenCache tokenCache = new TokenCache();

var result = CreateAdalResultWithIdToken(true);

// act
WriteMsalRefreshTokenValues(result, tokenCache.TokenCacheAccessor);

// assert
// IdToken present
Assert.AreEqual(tokenCache.TokenCacheAccessor.AccountCount, 1);
Assert.AreEqual(tokenCache.TokenCacheAccessor.RefreshTokenCount, 1);
Copy link
Contributor

@henrik-me henrik-me May 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Assert.AreEqual [](start = 12, length = 15)

can it be tested that the userinfo values are null? #Closed

Assert.AreEqual(AdalTestConstants.DefaultDisplayableId, result.Result.UserInfo.DisplayableId);
Assert.AreEqual(AdalTestConstants.DefaultUniqueId, result.Result.UserInfo.UniqueId);
}

private AdalResultWrapper CreateAdalResultWithIdToken(bool withIdToken)
{
var result = new AdalResultWrapper
{
RefreshToken = "some-rt",
RawClientInfo = MockHelpers.CreateClientInfo(AdalTestConstants.DefaultUniqueId, AdalTestConstants.DefaultUniqueTenantIdentifier),
ResourceInResponse = AdalTestConstants.DefaultResource
};

if (withIdToken)
{
result.Result = new AdalResult("Bearer", "some-access-token", DateTimeOffset.UtcNow, DateTimeOffset.UtcNow + TimeSpan.FromMinutes(180))
{
UserInfo = new AdalUserInfo()
{
DisplayableId = AdalTestConstants.DefaultDisplayableId,
UniqueId = AdalTestConstants.DefaultUniqueId
},
IdToken = MockHelpers.CreateAdalIdToken(
AdalTestConstants.DefaultUniqueId,
AdalTestConstants.DefaultDisplayableId)
};
}
else
{
result.Result = new AdalResult("Bearer", "some-access-token", DateTimeOffset.UtcNow, DateTimeOffset.UtcNow + TimeSpan.FromMinutes(180));
}

return result;
}

private void WriteMsalRefreshTokenValues(AdalResultWrapper result, ITokenCacheAccessor tokenCacheAccessor)
{
CacheFallbackOperations.WriteMsalRefreshToken(
tokenCacheAccessor,
result,
AdalTestConstants.DefaultAuthorityCommonTenant,
AdalTestConstants.DefaultClientId,
result.Result.UserInfo?.DisplayableId,
result.Result.UserInfo?.GivenName,
result.Result.UserInfo?.FamilyName,
result.Result.UserInfo?.UniqueId);
}
}
}
}