cache format tests + format changes #1350

SomkaPe · 2018-10-24T18:37:00Z

Cache format tests

While writing cache format test some cache format inconsistencies with current version of unified schema were found:

Timestamps should be stored as strings in Json
It was decided to not store client_info(which is optional field) in all cache entities - just in account
Credential Type field should be Case sensitive
Keys of cache entities should be in lower case (including iOS key chain key)
Scopes should not be stored in sorted form, instead should be stored as is (as returned from STS)
Change "preferred_username not in id_token" to "Missing from the token response"
Changing iOS key component "Generic" for account cache entity - store "username" instead of "local_account_id" (requested by Olga)

Others:

Persisting given_name and family_name optional claims in account cache entity
Fixing issue of client_info misuse:
2.1. AccounId is derived from ClientInfo (Not opposite way - ClientInfo can not be recreated from accountId, it can contains more fields )
2.2. Some code was written based on assumption that client info is always persisted with cache entities, which is wrong because it is optional field, so some library can not persist it.

…y-library-for-dotnet into somkape/cacheFormatTest

henrik-me · 2018-10-24T19:28:44Z

core/src/Cache/MsalCacheCommon.cs

-        public const string RefreshToken = "refreshtoken";
+        public const string IdToken = "IdToken";
+        public const string AccessToken = "AccessToken";
+        public const string RefreshToken = "RefreshToken";


not sure I understand why you are CamelCasing these. Though I see that the methods lowercases everything. #Pending

According to current schema design doc https://identitydivision.visualstudio.com/DevEx/DevEx%20Team/_git/AuthLibrariesApiReview?path=%2FUnifiedSchema%2FSchema.md&version=GBoldalton%2Funified_schema_transfer
credential_type is case sensitive, but all keys are lowercased

ok... not sure though why the values here are CamelCased (if it's being lower cased later anyway).

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

same values stored in cache entity in Camel Case but in corresponding key in lower case (it was agreed to have lower cased keys for all cache entities)

henrik-me · 2018-10-24T19:30:12Z

core/src/Cache/Items/MsalAccessTokenCacheItem.cs

@@ -60,8 +60,8 @@ internal MsalAccessTokenCacheItem
            NormalizedScopes = scopes;
            TenantId = tenantId;
            Secret = secret;
-            ExpiresOnUnixTimestamp = CoreHelpers.DateTimeToUnixTimestamp(accessTokenExpiresOn);
-            CachedAt = CoreHelpers.CurrDateTimeInUnixTimestamp();
+            ExpiresOnUnixTimestamp = CoreHelpers.DateTimeToUnixTimestamp(accessTokenExpiresOn).ToString();


Would it make sense to make the DateTimeToUnixTimestamp return as string or perhaps to add another helper e.g. DateTimeToUnixTimestampAsString ensuring people do not have to rememeber to add .ToString() ? #Closed

sure, will change #Closed

henrik-me · 2018-10-24T19:32:14Z

core/src/Cache/Items/MsalAccessTokenCacheItem.cs

@@ -45,7 +45,7 @@ internal MsalAccessTokenCacheItem()
        internal MsalAccessTokenCacheItem
            (string environment, string clientId, MsalTokenResponse response, string tenantId) :

-            this(environment, clientId, response.TokenType, ScopeHelper.ConvertStringToLowercaseSortedSet(response.Scope).AsSingleString(),
+            this(environment, clientId, response.TokenType, response.Scope,


response.Scope [](start = 60, length = 14)

Does this handle sorting and normalizing? I believe that is a requirement we wanted to ensure happened for the cache? #Pending

according to schema scopes should be stored as returned from sts

great, that was not how I understood it. Are we sure the server is sorting etc.?

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

no, it does not , we should do it during cache lookup logic

henrik-me · 2018-10-24T19:35:26Z

msal/tests/dev apps/DesktopTestApp/MsalUserAccessTokenControl.cs

@@ -35,7 +35,7 @@ public MsalUserAccessTokenControl()
        private void expireAccessTokenButton_Click(object sender, System.EventArgs e)
        {
            expiresOnLabel.Text = DateTimeOffset.UtcNow.ToString();
-            _item.ExpiresOnUnixTimestamp = CoreHelpers.DateTimeToUnixTimestamp(DateTimeOffset.UtcNow);
+            _item.ExpiresOnUnixTimestamp = CoreHelpers.DateTimeToUnixTimestamp(DateTimeOffset.UtcNow).ToString();


.ToString(); [](start = 101, length = 12)

made a comment elsewhere on this

henrik-me · 2018-10-24T19:36:42Z

msal/tests/dev apps/XForms/XForms/AccessTokenCacheItemDetails.xaml.cs

@@ -52,7 +53,7 @@ public partial class AccessTokenCacheItemDetails : ContentPage
            expiresOnLabel.Text = msalAccessTokenCacheItem.ExpiresOn.ToString();
            scopesLabel.Text = msalAccessTokenCacheItem.NormalizedScopes;

-            cachedAtLabel.Text = CoreHelpers.UnixTimestampToDateTime(msalAccessTokenCacheItem.CachedAt).ToString();
+            cachedAtLabel.Text = CoreHelpers.UnixTimestampToDateTime(Convert.ToInt64(msalAccessTokenCacheItem.CachedAt)).ToString();


Convert.ToInt64( [](start = 69, length = 16)

can this be moved to the MsalAccessTokenCacheItem class? #Closed

yes, will change #Closed

henrik-me · 2018-10-24T19:39:04Z

Thanks a lot for looking into this Petro! #Closed

henrik-me · 2018-10-24T19:39:43Z

{

for my education, are these test data the same across all libraries?

Refers to: msal/tests/Test.MSAL.NET.Unit/Resources/AADTestData.txt:1 in cc222be. [](commit_id = cc222be, deletion_comment = False)

henrik-me

🕐

jennyf19 · 2018-10-24T19:45:03Z

core/src/IdToken.cs

@@ -72,6 +74,12 @@ internal class IdToken
        [DataMember(Name = IdTokenClaim.HomeObjectId, IsRequired = false)]
        public string HomeObjectId { get; set; }

+        [DataMember(Name = IdTokenClaim.GivenName)]


this one doesn't need IsRequired = false?

it is optional, which means that other sdk might not persist it #ByDesign

I see. thanks.

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

jennyf19 · 2018-10-24T19:55:40Z

msal/src/Microsoft.Identity.Client/TokenCache.cs

@@ -56,7 +56,7 @@ namespace Microsoft.Identity.Client
    public sealed class TokenCache
 #pragma warning restore CS1574 // XML comment has cref attribute that could not be resolved
    {
-        internal const string NullPreferredUsernameDisplayLabel = "preferred_username not in id_token";
+        internal const string NullPreferredUsernameDisplayLabel = "Missing from the token response";


Missing from the token response [](start = 67, length = 31)

much better wording #Closed

SomkaPe · 2018-10-24T22:34:21Z

{

for my education, are these test data the same across all libraries?

Refers to: msal/tests/Test.MSAL.NET.Unit/Resources/AADTestData.txt:1 in cc222be. [](commit_id = cc222be, deletion_comment = False)

Yes Test cases can be found at VSTS at AuthLibrariesApiReview /UnifiedSchema/testcases #Closed

henrik-me · 2018-10-25T06:12:52Z

core/src/Cache/Keys/MsalAccountCacheKey.cs

        }

        public string GetiOSServiceKey()
        {
-            return _tenantId ?? "";
+            return (_tenantId ?? "").ToLower();


return (_tenantId ?? "").ToLower(); [](start = 7, length = 40)

I'm speculating, however wouldn't this righ backwards compatibility? #Closed

we do not access cache by individual key , we do it by cache entity type - like read all access tokens,
so this is not a problem #Closed

…y-library-for-dotnet into somkape/cacheFormatTest

jennyf19 · 2018-10-25T22:36:19Z

msal/tests/Test.MSAL.NET.Unit.net45/UnifiedCacheFormatTests.cs

@@ -0,0 +1,308 @@
+using Microsoft.Identity.Client;
+using Microsoft.Identity.Client.Internal;


Missing the header info

sure, will add

jennyf19 · 2018-10-25T22:37:48Z

core/src/Cache/Items/MsalAccessTokenCacheItem.cs


-        [DataMember(Name = "user_assertion_hash")]
+        [DataMember(Name = "user_assertion_hash", EmitDefaultValue = false)]


EmitDefaultValue = false [](start = 50, length = 24)

what's this for?

is used to determine whether to serialize the default value for a field or property being serialized

jennyf19 · 2018-10-25T22:38:34Z

core/src/Cache/Items/MsalCacheItemBase.cs

@@ -41,7 +41,6 @@ internal abstract class MsalCacheItemBase
        [DataMember(Name = "environment", IsRequired = true)]
        internal string Environment { get; set; }

-        [DataMember(Name = "client_info")]


remove this as a datamember because it's not required...correct? #Closed

it was decided to persist it only with account object #Closed

jennyf19 · 2018-10-25T22:43:11Z

msal/tests/Test.MSAL.NET.Unit.net45/UnifiedCacheFormatTests.cs

+        }
+    }
+}
+#endif


this is pretty cool. What about ADFS scenarios?

can be also included quite easily, main challenge is to create test data for adfs -> sts response and expectations , it probably should be done in separate PR related to enabling ADFS in msal

jennyf19 · 2018-10-25T22:45:06Z

core/src/Cache/Items/MsalAccountCacheItem.cs

        internal MsalAccountCacheKey GetKey()
        {
-            return new MsalAccountCacheKey(Environment, TenantId, HomeAccountId, LocalAccountId);
+            return new MsalAccountCacheKey(Environment, TenantId, HomeAccountId, PreferredUsername);


PreferredUsername [](start = 81, length = 17)

Why is this changing to PreferredUsername? Will that impact existing cache (pre-next release)?

this change was requested by iOS team, purpose is to optimize cache lookups,
it will not affect .NET Msal lookup because it does not perform lookup on individual keys

jennyf19 · 2018-10-25T22:45:57Z

core/src/Cache/Keys/MsalAccountCacheKey.cs

@@ -39,9 +39,9 @@ internal class MsalAccountCacheKey
        private readonly string _environment;
        private readonly string _homeAccountId;
        private readonly string _tenantId;
-        private readonly string _localAccountId;
+        private readonly string _username;


_username [](start = 32, length = 9)

why the switch to _username? #Closed

this field is used only for persistence on iOS - as value for "Generic", iOS team requested this change
to optimize cache lookup, from .Net side we need to change it to be compatible with iOS Msal #Closed

henrik-me · 2018-10-26T06:18:14Z

msal/tests/dev apps/XForms/XForms/CachePage.xaml.cs

@@ -111,7 +111,7 @@ public void OnExpire(object sender, EventArgs e)
            var tokenCache = App.MsalPublicClient.UserTokenCache;

            // set access token as expired
-            accessTokenCacheItem.ExpiresOnUnixTimestamp = GetCurrentTimestamp();
+            accessTokenCacheItem.ExpiresOnUnixTimestamp = GetCurrentTimestamp().ToString();


GetCurrentTimestamp [](start = 58, length = 19)

nit: consider making the method return a string

henrik-me · 2018-10-26T06:20:50Z

core/src/Cache/Items/MsalAccessTokenCacheItem.cs

@@ -61,7 +61,7 @@ internal MsalAccessTokenCacheItem
            TenantId = tenantId;
            Secret = secret;
            ExpiresOnUnixTimestamp = CoreHelpers.DateTimeToUnixTimestamp(accessTokenExpiresOn);
-            CachedAt = CoreHelpers.CurrDateTimeInUnixTimestamp();
+            CachedAt = CoreHelpers.CurrDateTimeInUnixTimestamp().ToString();


CurrDateTimeInUnixTimestamp(). [](start = 35, length = 30)

is this used anywhere else where it's supposed to return a long or can the method be updated to return a string?

henrik-me

…y-library-for-dotnet into somkape/cacheFormatTest

SomkaPe added 4 commits October 23, 2018 18:38

cache format test + fixes

5f1eaf2

Merge branch 'dev' of https://github.com/AzureAD/azure-activedirector…

668f82b

…y-library-for-dotnet into somkape/cacheFormatTest

minor fix

0c29521

minor fix

cc222be

SomkaPe requested review from jennyf19, henrik-me and MarkZuber October 24, 2018 19:16

henrik-me reviewed Oct 24, 2018

View reviewed changes

henrik-me suggested changes Oct 24, 2018

View reviewed changes

jennyf19 reviewed Oct 24, 2018

View reviewed changes

SomkaPe added 2 commits October 24, 2018 19:11

adding test for iOS compound key

f9d71ac

minor fix

e2e6f05

henrik-me reviewed Oct 25, 2018

View reviewed changes

SomkaPe requested a review from bgavrilMS October 25, 2018 18:44

Merge branch 'dev' of https://github.com/AzureAD/azure-activedirector…

0cebc93

…y-library-for-dotnet into somkape/cacheFormatTest

jennyf19 reviewed Oct 25, 2018

View reviewed changes

addressing pr comments

5ba79eb

henrik-me reviewed Oct 26, 2018

View reviewed changes

henrik-me approved these changes Oct 26, 2018

View reviewed changes

SomkaPe added 3 commits October 26, 2018 14:34

addressing pr comments

c6e77d6

minor fix

d74e755

Merge branch 'dev' of https://github.com/AzureAD/azure-activedirector…

7d37965

…y-library-for-dotnet into somkape/cacheFormatTest

SomkaPe merged commit 37b0eb5 into dev Oct 26, 2018

jmprieur mentioned this pull request Nov 7, 2018

change logs for next release #1389

Merged

SomkaPe deleted the somkape/cacheFormatTest branch December 13, 2018 18:26

		@@ -0,0 +1,308 @@
		using Microsoft.Identity.Client;
		using Microsoft.Identity.Client.Internal;


		[DataMember(Name = "user_assertion_hash")]
		[DataMember(Name = "user_assertion_hash", EmitDefaultValue = false)]

cache format tests + format changes #1350

cache format tests + format changes #1350

Conversation

SomkaPe commented Oct 24, 2018 • edited Loading

henrik-me Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrik-me Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

SomkaPe Oct 24, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrik-me Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

SomkaPe Oct 24, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me commented Oct 24, 2018 • edited Loading

henrik-me commented Oct 24, 2018

henrik-me left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SomkaPe Oct 24, 2018 • edited by jennyf19 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennyf19 Oct 24, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 24, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe commented Oct 24, 2018 • edited by henrik-me Loading

henrik-me Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

SomkaPe Oct 25, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennyf19 Oct 25, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 25, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennyf19 Oct 25, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 26, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrik-me left a comment

Choose a reason for hiding this comment

SomkaPe commented Oct 24, 2018 •

edited

Loading

henrik-me Oct 24, 2018 •

edited

Loading

henrik-me Oct 24, 2018 •

edited

Loading

SomkaPe Oct 24, 2018 •

edited by henrik-me

Loading

henrik-me Oct 24, 2018 •

edited

Loading

henrik-me Oct 24, 2018 •

edited

Loading

SomkaPe Oct 24, 2018 •

edited by henrik-me

Loading

henrik-me commented Oct 24, 2018 •

edited

Loading

SomkaPe Oct 24, 2018 •

edited by jennyf19

Loading

jennyf19 Oct 24, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 24, 2018 •

edited by henrik-me

Loading

SomkaPe commented Oct 24, 2018 •

edited by henrik-me

Loading

henrik-me Oct 25, 2018 •

edited

Loading

SomkaPe Oct 25, 2018 •

edited by henrik-me

Loading

jennyf19 Oct 25, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 25, 2018 •

edited by henrik-me

Loading

jennyf19 Oct 25, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 26, 2018 •

edited by henrik-me

Loading