FOCI #957

bgavrilMS · 2019-03-08T18:44:08Z

Please review commit by commit, some are just refactoring.

bgavrilMS · 2019-03-08T18:48:32Z

src/Microsoft.Identity.Client/Platforms/Android/AndroidTokenCacheAccessor.cs

        }
-
-        /// <inheritdoc />
-        public int RefreshTokenCount => throw new NotImplementedException();


I decided to remove these from the interface because they are actually only implemented on the InMemory stuff and are only used for test. It's dangerous to leave here, as one of use could use it, test on .net (which works), then realize that on Android and iOS it throws a NotImplEx....

If you agree with this, you can ignore looking at the rest of the commit - it's just refactoring tests.

src/Microsoft.Identity.Client/Cache/Items/MsalAppMetadataCacheItem.cs

src/Microsoft.Identity.Client/Cache/Keys/MsalAccountCacheKey.cs

tests/Microsoft.Identity.Test.Unit.net45/CoreTests/CacheTests/MsalTokenCacheKeysTests.cs

src/Microsoft.Identity.Client/Cache/Items/MsalRefreshTokenCacheItem.cs

henrik-me · 2019-03-13T02:44:47Z

src/Microsoft.Identity.Client/Cache/Items/MsalAccountCacheItem.cs


 namespace Microsoft.Identity.Client.Cache.Items
 {
-    [DataContract]


[DataContract] [](start = 4, length = 14)

isn't this needed for MSAL v2 cache? The dictionary serializer?

I don't think so, none of the other cache items have this attribute.

src/Microsoft.Identity.Client/Cache/Items/MsalAppMetadataCacheItem.cs

src/Microsoft.Identity.Client/Cache/Keys/MsalAccessTokenCacheKey.cs

src/Microsoft.Identity.Client/Cache/Keys/MsalCacheKeys.cs

henrik-me · 2019-03-13T02:53:08Z

src/Microsoft.Identity.Client/PlatformsCommon/Interfaces/IFeatureFlags.cs

+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+// 
+// ------------------------------------------------------------------------------


what about using the new short format?

henrik-me

🕐

src/Microsoft.Identity.Client/Cache/StorageJsonValues.cs

bgavrilMS · 2019-03-14T22:09:50Z

tests/Microsoft.Identity.Test.Unit.net45/Resources/ExpectedTokenCache.json

@@ -0,0 +1,201 @@
+{


@rayluo - check out this serialized cache that contains both app metadata and an FRT. Let me know if the format isn't what you expect.

tests/Microsoft.Identity.Test.Unit.net45/Resources/ExpectedTokenCache.json

src/Microsoft.Identity.Client/TokenCache.cs

henrik-me · 2019-03-15T17:25:02Z

src/Microsoft.Identity.Client/TokenCache.cs

            SetServiceBundle(serviceBundle);
        }

+        private TokenCache(IPlatformProxy proxy)


private TokenCache(IPlatformProxy proxy) [](start = 8, length = 40)

Shouldn't this rather a helper similar to SetServiceBundle ?

tests/Microsoft.Identity.Test.Unit.net45/CacheTests/TokenCacheTests.cs

henrik-me · 2019-03-15T17:34:36Z

src/Microsoft.Identity.Client/Cache/Keys/MsalAppMetadataCacheKey.cs

+    /// <summary>
+    /// App metadata is an optional entity in cache and can be used by apps to store additional metadata applicable to a particular client.
+    /// </summary>
+    internal class MsalAppMetadataCacheKey : IiOSKey


MsalAppMetadataCacheKey [](start = 19, length = 23)

Is this in sync with what iOS native does for the storage?

Yes, Olga documented this pretty well. Also, the feature flag for FOCI for iOS and Android is set to false, meaning that:

we won't serialize app metadata

we won't serialize the FamilyId field in the RT

we won't make cache calls to check AppMetadat or FRT

Let me know if this makes sense - I tried to optimize for performance, i.e. eliminate cache calls.
I can easily enable FOCI on iOS and Android (just a few simple methods to implement). On iOS, it should just work because the Cache Schema is documented. On Android not so much, but we can just choose a location for appmetadata and be done with it.

henrik-me · 2019-03-15T17:44:21Z

Has it been validated that this implementation is not causing issues in our cache sharing with iOS native?

bgavrilMS · 2019-03-16T19:50:01Z

@henrik-me , @MarkZuber

This is now complete. If you find that there are too many files to review, I suggest you review: TokenCache, SilentRequest and FociTests - the rest should be uncontroversial.

henrik-me · 2019-03-18T18:30:10Z

src/Microsoft.Identity.Client/Cache/Keys/IiOSKey.cs

@@ -0,0 +1,13 @@
+namespace Microsoft.Identity.Client.Cache.Keys
+{
+    internal interface IiOSKey


IiOSKey [](start = 23, length = 7)

IMsalIOSCacheKey ?

henrik-me · 2019-03-18T18:31:27Z

Generally looks good. Let's have a quick sync in person.

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

bgavrilMS commented Mar 8, 2019

View reviewed changes

MarkZuber reviewed Mar 8, 2019

View reviewed changes

src/Microsoft.Identity.Client/Cache/Items/MsalAppMetadataCacheItem.cs Outdated Show resolved Hide resolved

MarkZuber reviewed Mar 8, 2019

View reviewed changes

src/Microsoft.Identity.Client/Cache/Keys/MsalAccountCacheKey.cs Show resolved Hide resolved

MarkZuber reviewed Mar 8, 2019

View reviewed changes

tests/Microsoft.Identity.Test.Unit.net45/CoreTests/CacheTests/MsalTokenCacheKeysTests.cs Outdated Show resolved Hide resolved

bgavrilMS commented Mar 10, 2019

View reviewed changes

src/Microsoft.Identity.Client/Cache/Items/MsalRefreshTokenCacheItem.cs Show resolved Hide resolved

bgavrilMS force-pushed the bogavril/foci branch from d74060c to 9ca5d71 Compare March 10, 2019 17:38

henrik-me reviewed Mar 13, 2019

View reviewed changes

src/Microsoft.Identity.Client/Cache/Items/MsalAppMetadataCacheItem.cs Show resolved Hide resolved

henrik-me reviewed Mar 13, 2019

View reviewed changes

src/Microsoft.Identity.Client/Cache/Keys/MsalAccessTokenCacheKey.cs Show resolved Hide resolved

henrik-me reviewed Mar 13, 2019

View reviewed changes

src/Microsoft.Identity.Client/Cache/Keys/MsalCacheKeys.cs Show resolved Hide resolved

henrik-me reviewed Mar 13, 2019

View reviewed changes

MarkZuber added v3 labels Mar 13, 2019

henrik-me requested changes Mar 14, 2019

View reviewed changes

henrik-me reviewed Mar 14, 2019

View reviewed changes

src/Microsoft.Identity.Client/Cache/StorageJsonValues.cs Outdated Show resolved Hide resolved

bgavrilMS commented Mar 14, 2019

View reviewed changes

rayluo reviewed Mar 14, 2019

View reviewed changes

tests/Microsoft.Identity.Test.Unit.net45/Resources/ExpectedTokenCache.json Show resolved Hide resolved

henrik-me reviewed Mar 15, 2019

View reviewed changes

src/Microsoft.Identity.Client/TokenCache.cs Outdated Show resolved Hide resolved

henrik-me reviewed Mar 15, 2019

View reviewed changes

tests/Microsoft.Identity.Test.Unit.net45/CacheTests/TokenCacheTests.cs Outdated Show resolved Hide resolved

henrik-me reviewed Mar 15, 2019

View reviewed changes

tests/Microsoft.Identity.Test.Unit.net45/CacheTests/TokenCacheTests.cs Show resolved Hide resolved

henrik-me reviewed Mar 15, 2019

View reviewed changes

bgavrilMS added 7 commits March 16, 2019 17:49

Remove unused dev app (SampleApp)

f29f14b

Add netfx console sample app that exercises foci

b3d3ad8

Add AppMetadata to token cache obj model + ios key refactoring

a7d6fdc

AppMetadata serialization support

f0e685a

Add support for FRT

5777055

Add support for feature flags

cbf87ff

Create app metadata when receiving token responses

196b1db

bgavrilMS added 5 commits March 16, 2019 18:46

Fix a serialization issue and add better assert in ut

f94f946

Token cache changes for saving AppMetadada and FRT

36fd99b

Do not write FRTs to ADAL legacy cache

e60b558

SilentRequest logic and E2E FOCI tests

e0c1289

Update test app with non-foci app

87a44c1

bgavrilMS force-pushed the bogavril/foci branch from 98eca64 to 87a44c1 Compare March 16, 2019 18:49

Foci - minor rebase fixes and PR comments

12ac5c1

bgavrilMS requested a review from henrik-me March 16, 2019 19:50

henrik-me reviewed Mar 18, 2019

View reviewed changes

bgavrilMS merged commit f0bc7e0 into dev3x Mar 19, 2019

bgavrilMS deleted the bogavril/foci branch March 19, 2019 01:39

FOCI #957

FOCI #957

Uh oh!

Conversation

bgavrilMS commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgavrilMS Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henrik-me Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Mar 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henrik-me Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

henrik-me left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bgavrilMS Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

henrik-me Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

henrik-me Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

henrik-me commented Mar 15, 2019

Uh oh!

bgavrilMS commented Mar 16, 2019

Uh oh!

henrik-me Mar 18, 2019

Choose a reason for hiding this comment

Uh oh!

henrik-me commented Mar 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bgavrilMS commented Mar 8, 2019 •

edited

Loading

bgavrilMS Mar 8, 2019 •

edited

Loading

henrik-me Mar 15, 2019 •

edited

Loading