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

cache format tests + format changes #1350

Merged
merged 11 commits into from
Oct 26, 2018
Merged

cache format tests + format changes #1350

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5f1eaf2
cache format test + fixes
SomkaPe Oct 24, 2018
668f82b
Merge branch 'dev' of https://github.com/AzureAD/azure-activedirector…
SomkaPe Oct 24, 2018
0c29521
minor fix
SomkaPe Oct 24, 2018
cc222be
minor fix
SomkaPe Oct 24, 2018
f9d71ac
adding test for iOS compound key
SomkaPe Oct 25, 2018
e2e6f05
minor fix
SomkaPe Oct 25, 2018
0cebc93
Merge branch 'dev' of https://github.com/AzureAD/azure-activedirector…
SomkaPe Oct 25, 2018
5ba79eb
addressing pr comments
SomkaPe Oct 26, 2018
c6e77d6
addressing pr comments
SomkaPe Oct 26, 2018
d74e755
minor fix
SomkaPe Oct 26, 2018
7d37965
Merge branch 'dev' of https://github.com/AzureAD/azure-activedirector…
SomkaPe Oct 26, 2018
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
3 changes: 2 additions & 1 deletion core/src/Cache/CacheFallbackOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ public static void WriteMsalRefreshToken(ITokenCacheAccessor tokenCacheAccessor,
tokenCacheAccessor.SaveRefreshToken(rtItem);

MsalAccountCacheItem accountCacheItem = new MsalAccountCacheItem
(new Uri(authority).Host, objectId, resultWrapper.RawClientInfo, null, displayableId, resultWrapper.Result.TenantId);
(new Uri(authority).Host, objectId, resultWrapper.RawClientInfo, null, displayableId, resultWrapper.Result.TenantId,
givenName, familyName);
tokenCacheAccessor.SaveAccount(accountCacheItem);
}
catch (Exception ex)
Expand Down
14 changes: 7 additions & 7 deletions core/src/Cache/Items/MsalAccessTokenCacheItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

@henrik-me henrik-me Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to schema scopes should be stored as returned from sts

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

tenantId, response.AccessToken, response.AccessTokenExpiresOn, response.ClientInfo)
{
}
Expand All @@ -60,8 +60,8 @@ internal MsalAccessTokenCacheItem
NormalizedScopes = scopes;
TenantId = tenantId;
Secret = secret;
ExpiresOnUnixTimestamp = CoreHelpers.DateTimeToUnixTimestamp(accessTokenExpiresOn);
CachedAt = CoreHelpers.CurrDateTimeInUnixTimestamp();
ExpiresOnUnixTimestamp = CoreHelpers.DateTimeToUnixTimestamp(accessTokenExpiresOn).ToString();
Copy link
Contributor

@henrik-me henrik-me Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@SomkaPe SomkaPe Oct 24, 2018 •
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.

sure, will change #Closed

CachedAt = CoreHelpers.CurrDateTimeInUnixTimestamp().ToString();
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

RawClientInfo = rawClientInfo;

InitUserIdentifier();
Expand All @@ -78,12 +78,12 @@ internal MsalAccessTokenCacheItem
internal string NormalizedScopes { get; set; }

[DataMember(Name = "cached_at", IsRequired = true)]
internal long CachedAt { get; set; }
internal string CachedAt { get; set; }

[DataMember(Name = "expires_on", IsRequired = true)]
internal long ExpiresOnUnixTimestamp { get; set; }
internal string ExpiresOnUnixTimestamp { get; set; }

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

Choose a reason for hiding this comment

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

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

what's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

public string UserAssertionHash { get; set; }

[DataMember(Name = "access_token_type")]
Expand All @@ -109,7 +109,7 @@ internal DateTimeOffset ExpiresOn
get
{
DateTime dtDateTime = new DateTime(1970, 1, 1, 0, 0, 0, 0, DateTimeKind.Utc);
return dtDateTime.AddSeconds(ExpiresOnUnixTimestamp).ToUniversalTime();
return dtDateTime.AddSeconds(Convert.ToInt64(ExpiresOnUnixTimestamp)).ToUniversalTime();
}
}

Expand Down
28 changes: 21 additions & 7 deletions core/src/Cache/Items/MsalAccountCacheItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,31 +40,35 @@ internal MsalAccountCacheItem(string environment, MsalTokenResponse response) :
{
IdToken idToken = IdToken.Parse(response.IdToken);

Init(environment, idToken?.ObjectId, response.ClientInfo, idToken.Name, idToken.PreferredUsername, idToken.TenantId);
Init(environment, idToken?.ObjectId, response.ClientInfo, idToken.Name, idToken.PreferredUsername, idToken.TenantId,
idToken.GivenName, idToken.FamilyName);
}

internal MsalAccountCacheItem(string environment, MsalTokenResponse response, string preferredUsername, string tenantID) : this()
{
IdToken idToken = IdToken.Parse(response.IdToken);

Init(environment, idToken?.ObjectId, response.ClientInfo, idToken.Name, preferredUsername, tenantID);
Init(environment, idToken?.ObjectId, response.ClientInfo, idToken.Name, preferredUsername, tenantID,
idToken.GivenName, idToken.FamilyName);
}

internal MsalAccountCacheItem(string environment, string localAccountId, string rawClientInfo,
string name, string preferredUsername, string tenantId) : this()
string name, string preferredUsername, string tenantId, string givenName, string familyName) : this()
{
Init(environment, localAccountId, rawClientInfo, name, preferredUsername, tenantId);
Init(environment, localAccountId, rawClientInfo, name, preferredUsername, tenantId, givenName, familyName);
}

private void Init(string environment, string localAccountId, string rawClientInfo,
string name, string preferredUsername, string tenantId)
string name, string preferredUsername, string tenantId, string givenName, string familyName)
{
Environment = environment;
PreferredUsername = preferredUsername;
Name = name;
TenantId = tenantId;
LocalAccountId = localAccountId;
RawClientInfo = rawClientInfo;
GivenName = givenName;
FamilyName = familyName;

InitUserIdentifier();
}
Expand All @@ -78,10 +82,10 @@ private void Init(string environment, string localAccountId, string rawClientInf
[DataMember(Name = "name")]
internal string Name { get; set; }

[DataMember(Name = "given_name")]
[DataMember(Name = "given_name", EmitDefaultValue = false)]
internal string GivenName { get; set; }

[DataMember(Name = "family_name")]
[DataMember(Name = "family_name", EmitDefaultValue = false)]
internal string FamilyName { get; set; }

[DataMember(Name = "local_account_id")]
Expand All @@ -90,6 +94,16 @@ private void Init(string environment, string localAccountId, string rawClientInf
[DataMember(Name = "authority_type")]
internal string AuthorityType { get; set; }

[DataMember(Name = "client_info")]
internal string ClientInfoString
{
get => RawClientInfo;
set
{
RawClientInfo = value;
}
}

internal MsalAccountCacheKey GetKey()
{
return new MsalAccountCacheKey(Environment, TenantId, HomeAccountId, LocalAccountId);
Expand Down
1 change: 0 additions & 1 deletion core/src/Cache/Items/MsalCacheItemBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ internal abstract class MsalCacheItemBase
[DataMember(Name = "environment", IsRequired = true)]
internal string Environment { get; set; }

[DataMember(Name = "client_info")]
Copy link
Contributor

@jennyf19 jennyf19 Oct 25, 2018 •
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.

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

Copy link
Contributor Author

@SomkaPe SomkaPe Oct 25, 2018 •
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.

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

internal string RawClientInfo { get; set; }

internal ClientInfo ClientInfo {
Expand Down
2 changes: 1 addition & 1 deletion core/src/Cache/Keys/MsalAccessTokenCacheKey.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public override string ToString()
MsalCacheCommon.AccessToken,
_clientId,
_tenantId,
_normalizedScopes);
_normalizedScopes);
}

#region UWP
Expand Down
14 changes: 7 additions & 7 deletions core/src/Cache/MsalCacheCommon.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ internal class MsalCacheCommon
public const string ScopesDelimiter = " ";
public const string CacheKeyDelimiter = "-";

public const string IdToken = "idtoken";
public const string AccessToken = "accesstoken";
public const string RefreshToken = "refreshtoken";
public const string IdToken = "IdToken";
public const string AccessToken = "AccessToken";
public const string RefreshToken = "RefreshToken";
Copy link
Contributor

@henrik-me henrik-me Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)


public static string GetCredentialKey(string homeAccountId, string environment, string keyDescriptor, string clientId, string tenantId, string scopes)
{
Expand All @@ -63,7 +63,7 @@ public static string GetCredentialKey(string homeAccountId, string environment,

stringBuilder.Append(scopes ?? "");

return stringBuilder.ToString();
return stringBuilder.ToString().ToLower();
}

public static string GetiOSAccountKey(string homeAccountId, string environment)
Expand All @@ -75,7 +75,7 @@ public static string GetiOSAccountKey(string homeAccountId, string environment)

stringBuilder.Append(environment);

return stringBuilder.ToString();
return stringBuilder.ToString().ToLower();
}


Expand All @@ -94,7 +94,7 @@ public static string GetiOSServiceKey(string keyDescriptor, string clientId, str

stringBuilder.Append(scopes ?? "");

return stringBuilder.ToString();
return stringBuilder.ToString().ToLower();
}

public static string GetiOSGenericKey(string keyDescriptor, string clientId, string tenantId)
Expand All @@ -109,7 +109,7 @@ public static string GetiOSGenericKey(string keyDescriptor, string clientId, str

stringBuilder.Append(tenantId ?? "");

return stringBuilder.ToString();
return stringBuilder.ToString().ToLower();
}
}
}
2 changes: 1 addition & 1 deletion core/src/Cache/cache.readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Details in the [wiki](https://github.com/AzureAD/azure-activedirectory-library-f
### B2C notes

ADAL does not support B2C and there are no plans to add B2C support. As such, MSAL will not write B2C tokens to the ADAL token cache.
In B2C, currently, the displayName (aka username aka preferred username) is null. This is a "bug" in B2C as they should provide a scope for the username. DisplayName should never be null - it would be a schema violation for it to be null. We have code that adds a constant (smth like "preferred_username not in id_token") in these cases.
In B2C, currently, the displayName (aka username aka preferred username) is null. This is a "bug" in B2C as they should provide a scope for the username. DisplayName should never be null - it would be a schema violation for it to be null. We have code that adds a constant (smth like "Missing from the token response") in these cases.

### Account removal algorithm

Expand Down
8 changes: 8 additions & 0 deletions core/src/IdToken.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ internal class IdTokenClaim
public const string PreferredUsername = "preferred_username";
public const string Name = "name";
public const string HomeObjectId = "home_oid";
public const string GivenName = "given_name";
public const string FamilyName = "family_name";
}

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

[DataMember(Name = IdTokenClaim.GivenName)]
Copy link
Contributor

Choose a reason for hiding this comment

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

this one doesn't need IsRequired = false?

Copy link
Contributor Author

@SomkaPe SomkaPe Oct 24, 2018 •
edited by jennyf19
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. thanks.

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

public string GivenName { get; set; }

[DataMember(Name = IdTokenClaim.FamilyName, IsRequired = false)]
public string FamilyName { get; set; }

public static IdToken Parse(string idToken)
{
if (string.IsNullOrEmpty(idToken))
Expand Down
6 changes: 3 additions & 3 deletions msal/src/Microsoft.Identity.Client/Account.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,14 @@ internal sealed class Account : IAccount
/// <summary>
/// Constructor
/// </summary>
/// <param name="homeAccountId">An object and tenant scoped id; can be null, for example when migrating the ADAL v3 cache</param>
/// <param name="homeAccountId">Home account id in "uid.utid" format; can be null, for example when migrating the ADAL v3 cache</param>
/// <param name="username">UPN style , can be null</param>
/// <param name="environment">Identity provider for this account, e.g. <c>login.microsoftonline.com</c></param>
public Account(AccountId homeAccountId, string username, string environment)
public Account(string homeAccountId, string username, string environment)
{
Username = username;
Environment = environment;
HomeAccountId = homeAccountId;
HomeAccountId = AccountId.ParseFromString(homeAccountId);
}

public string Username { get; internal set; }
Expand Down
26 changes: 5 additions & 21 deletions msal/src/Microsoft.Identity.Client/AccountId.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,33 +80,17 @@ public AccountId(string identifier, string objectId, string tenantId)
ValidateId();
}



#region Adapter to / from ClientInfo
internal static AccountId FromClientInfo(ClientInfo clientInfo)
internal static AccountId ParseFromString(string str)
{
if (clientInfo == null)
if (string.IsNullOrEmpty(str))
{
throw new ArgumentNullException(nameof(clientInfo));
return null;
}
string[] elements = str.Split('.');

return new AccountId(
clientInfo.ToAccountIdentifier(),
clientInfo.UniqueObjectIdentifier,
clientInfo.UniqueTenantIdentifier);
}

internal ClientInfo ToClientInfo()
{
return new ClientInfo()
{
UniqueObjectIdentifier = this.ObjectId,
UniqueTenantIdentifier = this.TenantId
};
return new AccountId(str, elements[0], elements[1]);
}

#endregion

/// <summary>
/// Two accounts are equal when their <see cref="Identifier"/> properties match
/// </summary>
Expand Down
2 changes: 1 addition & 1 deletion msal/src/Microsoft.Identity.Client/AuthenticationResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ internal AuthenticationResult(MsalAccessTokenCacheItem msalAccessTokenCacheItem,
_msalIdTokenCacheItem = msalIdTokenCacheItem;
if (_msalAccessTokenCacheItem.HomeAccountId != null)
{
Account = new Account(AccountId.FromClientInfo(_msalAccessTokenCacheItem.ClientInfo),
Account = new Account(_msalAccessTokenCacheItem.HomeAccountId,
_msalIdTokenCacheItem?.IdToken?.PreferredUsername, _msalAccessTokenCacheItem.Environment);
}
}
Expand Down
2 changes: 0 additions & 2 deletions msal/src/Microsoft.Identity.Client/Internal/Requests/AuthenticationRequestParameters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,6 @@ internal class AuthenticationRequestParameters

public IAccount Account { get; set; }

public ClientInfo ClientInfo { get; set; }

public UserAssertion UserAssertion { get; set; }

public bool IsClientCredentialRequest { get; set; } = false;
Expand Down
10 changes: 4 additions & 6 deletions msal/src/Microsoft.Identity.Client/Internal/Requests/InteractiveRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,18 +196,16 @@ private Uri CreateAuthorizationUri(bool addVerifier = false, bool addState = fal
requestParameters[OAuth2Parameter.LoginHint] = AuthenticationRequestParameters.Account.Username;
}

AuthenticationRequestParameters.ClientInfo = AuthenticationRequestParameters.Account.HomeAccountId.ToClientInfo();

if (!string.IsNullOrEmpty(AuthenticationRequestParameters.ClientInfo.UniqueObjectIdentifier))
if (AuthenticationRequestParameters.Account?.HomeAccountId?.ObjectId != null)
{
requestParameters[OAuth2Parameter.LoginReq] =
AuthenticationRequestParameters.ClientInfo.UniqueObjectIdentifier;
AuthenticationRequestParameters.Account.HomeAccountId.ObjectId;
}

if (!string.IsNullOrEmpty(AuthenticationRequestParameters.ClientInfo.UniqueTenantIdentifier))
if (!string.IsNullOrEmpty(AuthenticationRequestParameters.Account?.HomeAccountId?.TenantId))
{
requestParameters[OAuth2Parameter.DomainReq] =
AuthenticationRequestParameters.ClientInfo.UniqueTenantIdentifier;
AuthenticationRequestParameters.Account.HomeAccountId.TenantId;
}
}

Expand Down
10 changes: 5 additions & 5 deletions msal/src/Microsoft.Identity.Client/Internal/Requests/RequestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,10 @@ protected AuthenticationResult CacheTokenResponseAndCreateAuthenticationResult(M
fromServer = ClientInfo.CreateFromJson(msalTokenResponse.ClientInfo);
}

if (fromServer!= null && AuthenticationRequestParameters.ClientInfo != null)
if (fromServer!= null && AuthenticationRequestParameters?.Account?.HomeAccountId != null)
{
if (!fromServer.UniqueObjectIdentifier.Equals(AuthenticationRequestParameters.ClientInfo.UniqueObjectIdentifier, StringComparison.OrdinalIgnoreCase) ||
!fromServer.UniqueTenantIdentifier.Equals(AuthenticationRequestParameters.ClientInfo.UniqueTenantIdentifier, StringComparison.OrdinalIgnoreCase))
if (!fromServer.UniqueObjectIdentifier.Equals(AuthenticationRequestParameters.Account.HomeAccountId.ObjectId, StringComparison.OrdinalIgnoreCase) ||
!fromServer.UniqueTenantIdentifier.Equals(AuthenticationRequestParameters.Account.HomeAccountId.TenantId, StringComparison.OrdinalIgnoreCase))
{
AuthenticationRequestParameters.RequestContext.Logger.Error("Returned user identifiers do not match the sent user identifier");

Expand All @@ -222,8 +222,8 @@ protected AuthenticationResult CacheTokenResponseAndCreateAuthenticationResult(M
"Returned user identifiers (uid:{0} utid:{1}) does not match the sent user identifier (uid:{2} utid:{3})",
fromServer.UniqueObjectIdentifier,
fromServer.UniqueTenantIdentifier,
AuthenticationRequestParameters.ClientInfo.UniqueObjectIdentifier,
AuthenticationRequestParameters.ClientInfo.UniqueTenantIdentifier),
AuthenticationRequestParameters.Account.HomeAccountId.ObjectId,
AuthenticationRequestParameters.Account.HomeAccountId.TenantId),
string.Empty);

throw new MsalClientException(MsalError.UserMismatch, MsalErrorMessage.UserMismatchSaveToken);
Expand Down
Loading