Split authority #151

antrix1989 · 2018-06-15T18:47:48Z

No description provided.

oldalton · 2018-06-28T21:32:51Z

IdentityCore/IdentityCore.xcodeproj/project.pbxproj

@@ -9,13 +9,21 @@
 /* Begin PBXBuildFile section */
 		04930F851FEC8E6800FC4DCD /* MSIDAadAuthorityCache.m in Sources */ = {isa = PBXBuildFile; fileRef = 04930F831FEC8E6800FC4DCD /* MSIDAadAuthorityCache.m */; };
 		04930F8B1FEC8EB900FC4DCD /* MSIDAadAuthorityCache.m in Sources */ = {isa = PBXBuildFile; fileRef = 04930F831FEC8E6800FC4DCD /* MSIDAadAuthorityCache.m */; };
-		04930F8C1FEC8EC200FC4DCD /* MSIDAadAuthorityCache+TestUtil.m in Sources */ = {isa = PBXBuildFile; fileRef = 04930F881FEC8E9400FC4DCD /* MSIDAadAuthorityCache+TestUtil.m */; };
-		04930F8D1FEC8EC200FC4DCD /* MSIDAadAuthorityCache+TestUtil.m in Sources */ = {isa = PBXBuildFile; fileRef = 04930F881FEC8E9400FC4DCD /* MSIDAadAuthorityCache+TestUtil.m */; };
+		04930F8C1FEC8EC200FC4DCD /* (null) in Sources */ = {isa = PBXBuildFile; };


Looks like some kind of a project file conflict (null). Can you check this?

oldalton · 2018-06-28T21:34:31Z

IdentityCore/src/MSIDAADNetworkConfiguration.h

+
+@property (nonatomic, nullable) NSString *aadAuthorityDiscoveryApiVersion;
+
+@property (nonatomic, nullable) NSString *drsDiscoveryApiVersion;


When do we use DRS for AAD authority validation?

we don't use it for AAD, it is for ADFS authority validation.

oldalton · 2018-06-28T21:35:04Z

IdentityCore/src/MSIDADFSType.h

+typedef NS_ENUM(NSInteger, MSIDADFSType)
+{
+    MSIDADFSTypeOnPrems,
+    MSIDADFSTypeCloud


Let's rename MSIDADFSTypeCloud to something more appropriate

oldalton · 2018-06-28T21:35:23Z

IdentityCore/src/MSIDADFSType.h

+
+typedef NS_ENUM(NSInteger, MSIDADFSType)
+{
+    MSIDADFSTypeOnPrems,


nit: OnPrems -> OnPrem

oldalton · 2018-06-28T21:37:07Z

IdentityCore/src/cache/accessor/MSIDLegacyTokenCacheAccessor.m

-        account.authority = [MSIDAuthority cacheUrlForAuthority:refreshToken.authority tenantId:refreshToken.realm];
+
+        __auto_type authorityFactory = [MSIDAuthorityFactory new];
+        __auto_type authority = [authorityFactory authorityFromUrl:refreshToken.authority rawTenant:refreshToken.realm context:nil error:nil];


will this do authority aliasing?

oldalton · 2018-06-29T00:00:26Z

IdentityCore/src/validation/MSIDAdfsAuthorityResolver.m

+{
+    if (!validate)
+    {
+        __auto_type openIdConfigurationEndpoint = [self openIdConfigurationEndpointForAuthority:authority.url];


Not all ADFS versions support openid

what should we do in such case? In current ADAL/MSAL we assume that adfs is always supports open id.

oldalton · 2018-06-29T00:01:44Z

IdentityCore/src/validation/MSIDAdfsAuthorityResolver.m

+         drsCloudRequest.context = context;
+         [drsCloudRequest sendWithBlock:^(id response, NSError *error)
+          {
+              if (response)


should we return error if there's no response?

oldalton · 2018-06-29T00:03:21Z

IdentityCore/src/validation/MSIDAuthority.m

-        return [@"adfs" isEqualToString:tenant];
-    }
-    return NO;
+    return [s_trustedHostList containsObject:host.lowercaseString];


if it's a base MSIDAuthority class, AAD authorities shouldn't be known

oldalton · 2018-06-29T00:06:47Z

IdentityCore/src/validation/MSIDB2CAuthorityResolver.m

+
+    if (validate && ![authority isKnown])
+    {
+        __auto_type error = MSIDCreateError(MSIDErrorDomain, MSIDErrorServerInvalidRequest, @"Authority validation is not supported for this type of authority", nil, nil, nil, context.correlationId, nil);


B2C should also have openid configuration endpoint. Can we reuse the same logic here?

I suggest to merge current pr first, and then we can add opend id endpoint support to B2C. Currently MSAL doesn't support it anyway.

oldalton · 2018-06-29T00:07:29Z

IdentityCore/src/webview/embeddedWebview/MSIDOAuth2EmbeddedWebviewController.m

@@ -191,7 +192,10 @@ - (void)webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigati
 {
    NSURL *requestURL = navigationAction.request.URL;

-    MSID_LOG_VERBOSE(self.context, @"-decidePolicyForNavigationAction host: %@", [MSIDAuthority isKnownHost:requestURL] ? requestURL.host : @"unknown host");
+//    __auto_type authorityFactory = [MSIDAuthorityFactory new];
+//    __auto_type authority = [authorityFactory authorityFromUrl:account.authority rawTenant:response.idTokenObj.realm context:nil error:nil];


nit: remove this

oldalton

Added a couple of comments. I'll do a more thorough pass after this PR and #130 get the latest update from dev.

oldalton · 2018-08-28T02:59:21Z

IdentityCore/src/MSIDAADNetworkConfiguration.m

@@ -33,6 +35,14 @@ + (void)initialize
    if (self == [MSIDAADNetworkConfiguration self])
    {
        s_defaultConfiguration = [MSIDAADNetworkConfiguration new];
+
+        s_trustedHostList = [NSSet setWithObjects:MSIDTrustedAuthority,


please see a comment in another PR about making sure the list of authorities is up to date

oldalton · 2018-08-28T03:00:09Z

IdentityCore/src/MSIDConstants.h

+extern NSString * _Nonnull const MSID_OS_VER_KEY;//iOS/OSX version
+extern NSString * _Nonnull const MSID_DEVICE_MODEL_KEY;//E.g. iPhone 5S
+
+extern NSString * _Nonnull const MSIDTrustedAuthority;


nit: can we follow the same format as other constants?

Decided to keep camel case constants and change upper case constants to camel case latter.

oldalton · 2018-08-28T03:07:16Z

IdentityCore/src/network/request_configurator/MSIDAADRequestConfigurator.m

+
+    if (authorityUrl)
+    {
+        requestUrl = [requestUrl msidURLForPreferredHost:[authorityUrl msidHostWithPortIfNecessary] context:nil error:nil];


So, what it does, it tries to replace the host of request url with host from network url. Can we just query for network host and swap the hosts? Or at least add a more thorough comment. This logic is a bit confusing to read.

oldalton · 2018-08-28T03:10:15Z

IdentityCore/src/validation/MSIDAADAuthority.m

+- (NSURL *)cacheUrlWithContext:(id<MSIDRequestContext>)context
+{
+    __auto_type universalAuthorityURL = [self universalAuthorityURL];
+    __auto_type authority = (MSIDAADAuthority *)[self.authorityFactory authorityFromUrl:universalAuthorityURL context:context error:nil];


nit: why are we ignoring error?

authorityFactory internally will write to logs if parsing has failed.

oldalton · 2018-08-28T03:13:36Z

IdentityCore/src/validation/MSIDAADAuthority.m

+
+- (nonnull NSString *)authorityType
+{
+    return MSID_TELEMETRY_VALUE_AUTHORITY_AAD;


nit: it's a bit weird to use a telemetry constant for authority type here. Can we rename the constant or rename the method to telemetryAuthorityType?

renamed to telemetryAuthorityType

oldalton · 2018-08-28T03:20:03Z

IdentityCore/src/validation/MSIDAuthority.h

-+ (BOOL)isADFSInstanceURL:(nonnull NSURL *)endpointUrl;
-+ (BOOL)isConsumerInstanceURL:(nonnull NSURL *)authorityURL;
-+ (BOOL)isB2CInstanceURL:(nonnull NSURL *)endpointUrl;
+- (nonnull NSArray<NSURL *> *)cacheAliases;


In the new cache we don't operate based on alias URLs, we operate based on environments which are strings. How can we now get a list of environment aliases?

We still can get them from MSIDOauth2Factory. On the other hand, I don't think that MSIDOauth2Factory is a right place for such logic, that is why I suggest to revise it latter, see #222.

oldalton · 2018-08-28T03:22:29Z

IdentityCore/src/validation/MSIDAuthorityFactory.m

+
+    if (error)
+    {
+        *error = MSIDCreateError(MSIDErrorDomain, MSIDErrorInvalidDeveloperParameter, @"Provided authority url is not a valid authority.", nil, nil, nil, context.correlationId, nil);


Should we create a base OIDC authority instead?
Let's create an issue to follow up... (don't block on this)

Probably we should. Created issue: #224

oldalton · 2018-08-28T03:26:33Z

IdentityCore/src/validation/MSIDAuthorityFactory.m

+        if (b2cAuthority) return b2cAuthority;
+    }
+
+    if ([MSIDAADAuthority isAuthorityFormatValid:url context:context error:nil])


this checks for MSIDAADAuthority and creates ADFS authority

oldalton · 2018-08-28T03:26:50Z

IdentityCore/src/validation/MSIDAuthorityFactory.m

+{
+    if ([MSIDB2CAuthority isAuthorityFormatValid:url context:context error:nil])
+    {
+        __auto_type b2cAuthority = [[MSIDB2CAuthority alloc] initWithURL:url rawTenant:rawTenant context:context error:nil];


none of the initializers takes error?

Added underlyingError.

oldalton · 2018-08-28T03:27:24Z

IdentityCore/src/validation/MSIDAuthorityFactory.m

+                               context:(id<MSIDRequestContext>)context
+                                 error:(NSError **)error
+{
+    if ([MSIDB2CAuthority isAuthorityFormatValid:url context:context error:nil])


this code looks quite similar to the next one except there's no tenant. Can we just pass nil tenant to initializer?

good point, refactored.

oldalton

Added a couple of more suggestions, but otherwise the changes look fine to me after comments are addressed. I'm also seeing a couple of issues that have been fixes in other PR, so we need to merge dev back into this PR.

oldalton · 2018-09-05T03:59:52Z

IdentityCore/src/MSIDAADNetworkConfiguration.m

+    {
+        s_defaultConfiguration = [MSIDAADNetworkConfiguration new];
+
+        s_trustedHostList = [NSSet setWithObjects:MSIDTrustedAuthority,


I think you added more hosts into here in another PR?

oldalton · 2018-09-05T04:00:35Z

IdentityCore/src/MSIDAADNetworkConfiguration.m

+
+- (NSSet<NSString *> *)trustedHosts
+{
+    return s_trustedHostList;


nit: remove ;

oldalton · 2018-09-05T04:01:08Z

IdentityCore/src/MSIDCache.m

+- (void)setObject:(id)obj forKey:(id)key
+{
+    dispatch_barrier_sync(self.synchronizationQueue, ^{
+        [self.container setObject:obj forKey:key];


I think you fixed this to use subscript in another PR?

yes, it was fixed in another PR.

oldalton · 2018-09-05T04:01:44Z

IdentityCore/src/MSIDOAuth2Constants.h

@@ -26,8 +26,6 @@ extern NSString *const MSID_OAUTH2_AUTHORIZATION;
 extern NSString *const MSID_OAUTH2_AUTHORIZATION_CODE;
 extern NSString *const MSID_OAUTH2_AUTHORIZATION_URI;
 extern NSString *const MSID_OAUTH2_AUTHORITY;
-extern NSString *const MSID_OAUTH2_AUTHORIZE_SUFFIX;


those constants are still used in ADAL. Please revert removal...

oldalton · 2018-09-05T04:02:44Z

IdentityCore/src/cache/accessor/MSIDDefaultTokenCacheAccessor.m

@@ -229,8 +231,8 @@ - (MSIDAccessToken *)getAccessTokenForAccount:(MSIDAccountIdentifier *)account

    MSIDDefaultCredentialCacheQuery *query = [MSIDDefaultCredentialCacheQuery new];
    query.homeAccountId = account.homeAccountId;
-    query.environment = configuration.authority.msidHostWithPortIfNecessary;
-    query.realm = configuration.authority.msidTenant;
+    query.environment = configuration.authority.url.msidHostWithPortIfNecessary;


maybe we can have "host" method in authority object itself? That way we can avoid recalculating it every time through msidHostWithPortIfNecessary.

oldalton · 2018-09-05T04:03:06Z

IdentityCore/src/cache/accessor/MSIDDefaultTokenCacheAccessor.m

-    query.environment = accessToken.authority.msidHostWithPortIfNecessary;
-    query.realm = accessToken.authority.msidTenant;
+    query.environment = accessToken.authority.url.msidHostWithPortIfNecessary;
+    query.realm = accessToken.authority.url.msidTenant;


don't we have tenant in authority object itself?

We have tenant in MSIDAADAuthority, but since authority here is MSIDAuthority, we can not get tenant without dynamic cast to MSIDAADAuthority.

…ing.

Default accessor fixes related to authority aliases

oldalton · 2018-09-12T00:06:56Z

IdentityCore/src/validation/MSIDAADAuthority.m

+- (id)copyWithZone:(NSZone *)zone
+{
+    MSIDAADAuthority *authority = [super copyWithZone:zone];
+    authority->_tenant = [_tenant copyWithZone:zone];


did it get done?

antrix1989 requested review from unpluggedk, jasoncoolmax and oldalton June 15, 2018 18:47

antrix1989 added 10 commits June 22, 2018 20:12

Split authorities.

e84b227

Merge branch 'dev' into sedemche/split_authorities

263cf6f

Fixes after merge conflict.

e4e2bc3

Refactor MSIDAadAuthorityCache.

94b7545

Use MSIDAuthority in MSIDAccount.

d4b1ded

Uncomment MSIDAADAuthorityTests.

346dd15

Normalize MSIDADFSAuthority.

14a75fe

Use universal url in cacheUrlWithContext.

3f3f925

Use universal url in cacheAliases.

77ee6d3

Code cleaning.

12739ce

antrix1989 force-pushed the sedemche/split_authorities branch from d34e91b to 12739ce Compare June 26, 2018 18:54

antrix1989 changed the title ~~Split authority [in progress]~~ Split authority Jun 27, 2018

antrix1989 added 2 commits June 27, 2018 15:03

Merge branch 'dev' into sedemche/split_authorities

423d24f

Fixes after merge.

7d57ca4

antrix1989 changed the base branch from sedemche/aad_authority_nscache to dev June 28, 2018 21:31

oldalton reviewed Jun 29, 2018

View reviewed changes

antrix1989 added 11 commits July 3, 2018 13:56

Fix project file.

5ade21d

Rename DRS enum.

b6ef418

Remove commented code.

8f245a9

Fix DRS discovery request.

374a7b9

Support NSCopying in MSIDCache.

33a5041

Fill in the error.

ee537b0

Pass host instead of url in aadAuthorityWithEnvironment.

10a8ada

Removed cache from public properties.

7ac3f98

Fix unit tests.

fe02792

Remove cacheURLForAuthority from factories.

d5457e3

Use MSIDAuthority in MSIDConfiguration.

d06fd38

antrix1989 changed the base branch from dev to sedemche/authorityAliases August 28, 2018 02:41

oldalton reviewed Aug 28, 2018

View reviewed changes

antrix1989 changed the base branch from sedemche/authorityAliases to dev August 29, 2018 00:27

antrix1989 changed the base branch from dev to sedemche/authorityAliases August 29, 2018 00:27

antrix1989 changed the base branch from sedemche/authorityAliases to dev September 4, 2018 19:00

oldalton reviewed Sep 5, 2018

View reviewed changes

antrix1989 and others added 21 commits September 5, 2018 17:15

Merge branch 'dev' into sedemche/split_authorities

3d1d0a6

Fix list of known hosts.

d2a1d30

Remove cacheURLForAuthority from MSIDOauth2Factory.

1991882

Remove legacyRefreshTokenLookupAuthorities from MSIDOauth2Factory.

9ccc679

Remove legacyAccessTokenLookupAuthorities from MSIDOauth2Factory.

666c64b

Code cleaning.

1434eb0

Rename authorityType -> telemetryAuthorityType.

c0b00df

Copy openIdConfigurationEndpoint.

11d74cc

Remove intitializer with url & tenant from base MSIDAuthroity class.

2785096

Fix validating MSIDADFSAuthority in MSIDAuthorityFactory.

ff5e718

Refactor MSIDAuthorityFactory.

26ebba1

Provide underlyingError in MSIDAuthorityFactory during authority pars…

94a7622

…ing.

Add 'environment' property to authority class.

8985b65

Merge branch 'dev' into sedemche/split_authorities

5bbda28

Verify for nil authority.

c9e0945

Fixed default accessor with authority aliases

5f33aa3

Fixed duplicate file issues

1308a1b

Fixed automation with new authorities

c517b89

Use authority factory in credential helper.

9b30681

Merge from sedemche/split_authorities

b14ff46

Merge pull request #225 from AzureAD/oldalton/authority_aliases_fixes

bbcb8c6

Default accessor fixes related to authority aliases

oldalton approved these changes Sep 12, 2018

View reviewed changes

antrix1989 merged commit 624cd6b into dev Sep 12, 2018

antrix1989 deleted the sedemche/split_authorities branch September 12, 2018 00:38


		@property (nonatomic, nullable) NSString *aadAuthorityDiscoveryApiVersion;

		@property (nonatomic, nullable) NSString *drsDiscoveryApiVersion;

Split authority #151

Split authority #151

Conversation

antrix1989 commented Jun 15, 2018

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

oldalton Jun 28, 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

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

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

oldalton left a 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

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

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

antrix1989 Sep 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oldalton left a 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

Choose a reason for hiding this comment

oldalton Sep 5, 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

Choose a reason for hiding this comment

oldalton Jun 28, 2018 •

edited

Loading

antrix1989 Sep 7, 2018 •

edited

Loading

oldalton Sep 5, 2018 •

edited

Loading