Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable unmarshaling partitioned caches #456

Closed
wants to merge 1 commit into from
Closed

Enable unmarshaling partitioned caches #456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 7 additions & 5 deletions apps/internal/base/internal/storage/items.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,13 @@ type Contract struct {
// the internal cache. This design is shared between MSAL versions in many languages.
// This cannot be changed without design that includes other SDKs.
type InMemoryContract struct {
AccessTokensPartition map[string]map[string]AccessToken
RefreshTokensPartition map[string]map[string]accesstokens.RefreshToken
IDTokensPartition map[string]map[string]IDToken
AccountsPartition map[string]map[string]shared.Account
AppMetaData map[string]AppMetaData
AccessTokensPartition map[string]map[string]AccessToken `json:"AccessTokensPartition,omitempty"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bgavrilMS is there a schema for partitioned caches or an example of how one should look serialized? I didn't find any, so I don't know the correct keys here.

Copy link
Member

Choose a reason for hiding this comment

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

The serialized partitioned cache should look exactly like the non-partitioned cache. The partitioned cache is just an implementation detail in MSAL, so that MSAL is able to perform fast look-up.

You do this by going through the entire double dictionary and fetching the tokens. The first "key" is the partition key (ignore it) and the second key is the access token key - identical to the non-partitioned cache.

E.g. for an app access token you can have:

partition key: clientId + tenantId1
access token key: clientId + tenantId1 + resource1 and clientId + tenantId1 + resource2

partition key: clientId + tenantId2
access token key: clientId + tenantId2 + resource1 and clientId + tenantId2 + resource2

But in the serialized cache you will have only the 4 tokens with their access token keys.

@pmaytak can help here as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean that MSAL has a collection of partitions, each partition is indistinguishable from a non-partitioned cache, and MSAL should de/serialize only one partition at a time? That is to say, the partition key is an input to de/serialize?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, partition key is a cache entry key to de/serialize.

Something like this:
image

See app cache implementation in MSAL.NET

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

MSAL Go partitions data in memory as expected however it always de/serializes all partitions. There's no way to de/serialize a single partition. Let's not proceed with this PR then, much more is required to make persistent caching work correctly with partitions.

Copy link
Contributor

Choose a reason for hiding this comment

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

MSAL.NET also de/serializes all partitions (so full internal cache). However, an L2 read only ever loads one partition. So as long as CCA is created per request, there's only ever 1 partition in the cache.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm missing something here. MSAL Go gives the storage implementation opaque bytes and a suggested partition key, implying the bytes represent a single partition. If MSAL always de/serializes all partitions, a storage implementation that divided the cache across physical storage according to MSAL's suggested keys would just create redundant copies of the entire cache, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

If MSAL always de/serializes all partitions, a storage implementation that divided the cache across physical storage according to MSAL's suggested keys would just create redundant copies of the entire cache, no?

Yes, if MSAL instance (with the internal cache) is used as a singleton - then the external cache entries can have duplicate data. That's why we tell to create a new confidential client instance per request, so that the cache is empty initially and is deserialized with one partition.

gives the storage implementation opaque bytes and a suggested partition key, implying the bytes represent a single partition

This should be how it works, but MSAL.NET's implementation was not ideal. Ideally the serializer will accept a cache key, and try to serialize only that.

RefreshTokensPartition map[string]map[string]accesstokens.RefreshToken `json:"RefreshTokensPartition,omitempty"`
IDTokensPartition map[string]map[string]IDToken `json:"IdTokensPartition,omitempty"`
AccountsPartition map[string]map[string]shared.Account `json:"AccountsPartition,omitempty"`
AppMetaData map[string]AppMetaData `json:"AppMetadata,omitempty"`

AdditionalFields map[string]interface{}
}

// NewContract is the constructor for Contract.
Expand Down
34 changes: 34 additions & 0 deletions apps/internal/base/internal/storage/partitioned_storage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package storage
import (
"context"
"fmt"
"os"
"reflect"
"testing"
"time"
Expand Down Expand Up @@ -253,6 +254,39 @@ func TestReadPartitionedAccount(t *testing.T) {
}
}

func TestUnmarshalPartitioned(t *testing.T) {
manager := newPartitionedManagerForTest(nil)
b, err := os.ReadFile("testdata/test_partitioned_cache.json")
if err != nil {
t.Fatal(err)
}
err = manager.Unmarshal(b)
if err != nil {
t.Fatal(err)
}
hash := "0EuY9I6Pi8wVxq5awFCAHNbc_UKPtfnmXE4W54BzQPo="
actual := manager.contract.AccessTokensPartition[hash]["uid.utid-login.windows.net-accesstoken-my_client_id-contoso-s2 s1 s3"].Secret
if actual != accessTokenSecret {
t.Errorf("got access token %q, want %q", actual, accessTokenSecret)
}
actual = manager.contract.RefreshTokensPartition[hash]["uid.utid-login.windows.net-refreshtoken-my_client_id--s2 s1 s3"].Secret
if actual != rtSecret {
t.Errorf("got refresh token %q, want %q", actual, rtSecret)
}
actual = manager.contract.IDTokensPartition[hash]["uid.utid-login.windows.net-idtoken-my_client_id-contoso-"].Secret
if actual != idSecret {
t.Errorf("got ID token %q, want %q", actual, idSecret)
}
actual = manager.contract.AccountsPartition[hash]["uid.utid-login.windows.net-contoso"].PreferredUsername
if actual != accUser {
t.Errorf("got username %q, want %q", actual, accUser)
}
actual = manager.contract.AppMetaData["appmetadata-login.windows.net-my_client_id"].FamilyID
if actual != "1" {
t.Errorf(`got family ID %q, want "1"`, actual)
}
}

func TestWritePartitionedAccount(t *testing.T) {
storageManager := newPartitionedManagerForTest(nil)
testAcc := shared.NewAccount("hid", "env", "realm", "lid", accAuth, "username")
Expand Down
61 changes: 61 additions & 0 deletions apps/internal/base/internal/storage/testdata/test_partitioned_cache.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
{
"AccountsPartition": {
"0EuY9I6Pi8wVxq5awFCAHNbc_UKPtfnmXE4W54BzQPo=": {
"uid.utid-login.windows.net-contoso": {
"username": "John Doe",
"local_account_id": "object1234",
"realm": "contoso",
"environment": "login.windows.net",
"home_account_id": "uid.utid",
"authority_type": "MSSTS"
}
}
},
"RefreshTokensPartition": {
"0EuY9I6Pi8wVxq5awFCAHNbc_UKPtfnmXE4W54BzQPo=": {
"uid.utid-login.windows.net-refreshtoken-my_client_id--s2 s1 s3": {
"target": "s2 s1 s3",
"environment": "login.windows.net",
"credential_type": "RefreshToken",
"secret": "a refresh token",
"client_id": "my_client_id",
"home_account_id": "uid.utid"
}
}
},
"AccessTokensPartition": {
"0EuY9I6Pi8wVxq5awFCAHNbc_UKPtfnmXE4W54BzQPo=": {
"uid.utid-login.windows.net-accesstoken-my_client_id-contoso-s2 s1 s3": {
"environment": "login.windows.net",
"credential_type": "AccessToken",
"secret": "an access token",
"realm": "contoso",
"target": "s2 s1 s3",
"client_id": "my_client_id",
"cached_at": "1000",
"home_account_id": "uid.utid",
"extended_expires_on": "4600",
"expires_on": "4600"
}
}
},
"IDTokensPartition": {
"0EuY9I6Pi8wVxq5awFCAHNbc_UKPtfnmXE4W54BzQPo=": {
"uid.utid-login.windows.net-idtoken-my_client_id-contoso-": {
"realm": "contoso",
"environment": "login.windows.net",
"credential_type": "IdToken",
"secret": "header.eyJvaWQiOiAib2JqZWN0MTIzNCIsICJwcmVmZXJyZWRfdXNlcm5hbWUiOiAiSm9obiBEb2UiLCAic3ViIjogInN1YiJ9.signature",
"client_id": "my_client_id",
"home_account_id": "uid.utid"
}
}
},
"AppMetadata": {
"appmetadata-login.windows.net-my_client_id": {
"environment": "login.windows.net",
"client_id": "my_client_id",
"family_id": "1"
}
}
}