Move authority migration to common core #130

antrix1989 · 2018-05-05T01:39:47Z

No description provided.

… & parsing.

oldalton · 2018-05-14T16:43:11Z

IdentityCore/src/MSIDOAuth2Constants.m

@@ -108,3 +107,17 @@
 NSString *const MSID_LEGACY_TOKEN_CACHE_TYPE             = @"legacysingleresourcetoken";
 NSString *const MSID_ID_TOKEN_CACHE_TYPE                 = @"idtoken";
 NSString *const MSID_GENERAL_TOKEN_CACHE_TYPE            = @"token";
+
+NSString *MSID_OAUTH2_AUTHORIZE_SUFFIX(void)


Can we move it out of constants?

I like the approach, but with Olga in this one, lets move it

oldalton · 2018-05-14T16:45:19Z

IdentityCore/src/network/request/MSIDAADGetAuthorityMetadataRequest.m

+        NSParameterAssert(authority);
+
+        NSMutableDictionary *parameters = [NSMutableDictionary new];
+        parameters[@"api-version"] = @"1.1";


nit: can this be configurable?

oldalton · 2018-05-14T16:46:04Z

IdentityCore/src/network/request/MSIDAADGetAuthorityMetadataRequest.m

+
+        NSMutableDictionary *parameters = [NSMutableDictionary new];
+        parameters[@"api-version"] = @"1.1";
+        __auto_type authorizationEndpoint = [authority.absoluteString.lowercaseString stringByAppendingString:MSID_OAUTH2_AUTHORIZE_SUFFIX()];


do we need to hardcode this one?

oldalton · 2018-05-14T16:46:55Z

IdentityCore/src/network/request/MSIDDRSDiscoveryRequest.m

+        return nil;
+    }
+
+    __auto_type endpoint = (NSString *)jsonObject[@"IdentityProviderService"][@"PassiveAuthEndpoint"];


Can we make sure it's coming as NSString?

oldalton · 2018-05-14T16:47:36Z

IdentityCore/src/network/request/MSIDDRSDiscoveryRequest.m

+    else if (type == MSIDADFSTypeCloud)
+    {
+        return [NSURL URLWithString:
+                [NSString stringWithFormat:@"https://enterpriseregistration.windows.net/%@/enrollmentserver/contract", domain.lowercaseString]];


does this have to be a hardcoded URL?

moved it out

oldalton · 2018-05-14T17:02:54Z

IdentityCore/src/validation/MSIDAuthority.m

+    }
+
+    // B2C
+    if ([self isB2CInstanceURL:authority])


consider adding a separate class for normalization and additional authority manipulations. e.g. separate for B2C, ADFS etc

oldalton · 2018-05-14T17:04:01Z

IdentityCore/src/validation/MSIDAuthority.m

+        return NO;
+    }
+
+    if (authority.pathComponents.count < 2)


again, this is realistic for AAD, but for other authorities there might be less than 2 components, because there's no tenant. Can we make sure we separate the "base" implementation from AAD specific things.

i agree with possible need for a specific aad check.
However, for check is for all authorities afaik. b2c will have "tfp" as a component, adfs => "adfs" and aad will have it's value.

oldalton · 2018-05-14T17:04:46Z

IdentityCore/src/validation/MSIDAuthorityCacheRecord.h

+
+#import <Foundation/Foundation.h>
+
+@interface MSIDAuthorityCacheRecord : NSObject


just curious, what is this class for?

It is used for saving authority validation info into the cache. We use MSIDAuthorityCacheRecord & MSIDAadAuthorityCacheRecord.

oldalton · 2018-05-14T17:05:19Z

IdentityCore/src/validation/MSIDB2CAuthorityResolver.m

+                  context:(id<MSIDRequestContext>)context
+          completionBlock:(MSIDAuthorityInfoBlock)completionBlock
+{
+    if (validate && ![MSIDAuthority isKnownHost:authority])


does this mean if authority validation is enabled for B2C, MSAL always fails?

That is correct. Not supported.

oldalton · 2018-05-14T17:05:41Z

IdentityCore/src/validation/MSIDOpenIdProviderMetadata.h

+
+@interface MSIDOpenIdProviderMetadata : NSObject
+
+@property (nonatomic) NSURL *authorizationEndpoint;


should we have additional fields from the protocol in here?

We can add additional fields, but we don't use them right now.

unpluggedk

Set of nitpicks in this review,
Will do a logic review shortly.

unpluggedk · 2018-05-14T18:04:39Z

IdentityCore/src/MSIDConfiguration.h

+
+#import <Foundation/Foundation.h>
+
+@interface MSIDConfiguration : NSObject


Remember we've moved request params to be a configuration.
Please rename this class.

unpluggedk · 2018-05-14T18:05:41Z

IdentityCore/src/MSIDOAuth2Constants.m

@@ -108,3 +107,17 @@
 NSString *const MSID_LEGACY_TOKEN_CACHE_TYPE             = @"legacysingleresourcetoken";
 NSString *const MSID_ID_TOKEN_CACHE_TYPE                 = @"idtoken";
 NSString *const MSID_GENERAL_TOKEN_CACHE_TYPE            = @"token";
+
+NSString *MSID_OAUTH2_AUTHORIZE_SUFFIX(void)


I like the approach, but with Olga in this one, lets move it

unpluggedk · 2018-05-14T18:07:05Z

IdentityCore/src/MSIDOAuth2Constants.m

+{
+    __auto_type apiVersion = MSIDConfiguration.defaultConfiguration.aadApiVersion;
+
+    return [NSString stringWithFormat:@"/oauth2/%@%@authorize", apiVersion ?: @"", apiVersion ? @"/" : @""];


ternary within a ternary is a hard read.

unpluggedk · 2018-05-14T18:07:14Z

IdentityCore/src/MSIDOAuth2Constants.m

+{
+    __auto_type apiVersion = MSIDConfiguration.defaultConfiguration.aadApiVersion;
+
+    return [NSString stringWithFormat:@"/oauth2/%@%@token", apiVersion ?: @"", apiVersion ? @"/" : @""];


same as above

unpluggedk · 2018-05-14T18:10:18Z

IdentityCore/src/network/request/MSIDAADGetAuthorityMetadataRequest.h

+
+@end
+
+@interface MSIDAADGetAuthorityMetadataRequest : MSIDHttpRequest


nit: Lets remove "Get" from the name

unpluggedk · 2018-05-14T18:11:37Z

IdentityCore/src/network/request/MSIDDRSDiscoveryRequest.h

+@interface MSIDDRSDiscoveryRequest : MSIDHttpRequest
+
+- (instancetype)initWithDomain:(NSString *)domain
+                        adfsType:(MSIDADFSType)adfsType NS_DESIGNATED_INITIALIZER;


nit: alignment

unpluggedk · 2018-05-14T18:36:53Z

IdentityCore/src/network/request/MSIDAADGetAuthorityMetadataRequest.m

+    NSString *oauthError = jsonObject[@"error"];
+    if (![NSString msidIsStringNilOrBlank:oauthError])
+    {
+        if (error) *error = MSIDCreateError(MSIDErrorDomain,


nit: this could use a bracket and new lines :p

unpluggedk · 2018-05-14T18:38:14Z

IdentityCore/src/network/request/MSIDDRSDiscoveryRequest.m

+    self = [super init];
+    if (self)
+    {
+        NSParameterAssert(domain);


If you are asserting this, might as well put nonnull at method declaration

unpluggedk · 2018-05-14T18:39:24Z

IdentityCore/src/network/request/MSIDWebFingerRequest.m

+        _parameters = parameters;
+
+        NSMutableURLRequest *urlRequest = [NSMutableURLRequest new];
+        urlRequest.URL = [NSURL URLWithString:[NSString stringWithFormat:@"https://%@/.well-known/webfinger", issuer.host]];


yes it is afaik

unpluggedk · 2018-05-14T21:50:46Z

IdentityCore/src/validation/MSIDAuthority.h

@@ -42,4 +54,24 @@
 + (NSURL *)cacheUrlForAuthority:(NSURL *)authority
                       tenantId:(NSString *)tenantId;

+ (void)discoverAuthority:(NSURL *)authority


discovery is a part of the process, not the overall definition of what we need to do.

unpluggedk · 2018-05-14T21:51:32Z

IdentityCore/src/validation/MSIDAuthority.h

+                            context:(id<MSIDRequestContext>)context
+                    completionBlock:(MSIDOpenIdConfigurationInfoBlock)completionBlock;
+
+ (NSURL *)normalizeAuthority:(NSURL *)authority


Why accept URL not string as a parameter here?

We use authority as NSURL everywhere in common core. In my opinion, in order to be consistent, we should use it as NSURL here as well.

unpluggedk · 2018-05-14T21:52:19Z

IdentityCore/src/validation/MSIDAuthority.h

+                      context:(id<MSIDRequestContext>)context
+                        error:(NSError * __autoreleasing *)error;
+
+ (BOOL)isAuthorityFormatValid:(NSURL *)authority


What does this serve that normalizeAuthority doesn't? i.e./ does this need to be exposed outside of this class?

Removed it's declaration from public header.

unpluggedk · 2018-05-14T21:53:49Z

IdentityCore/src/validation/MSIDAuthority.h

 @interface MSIDAuthority : NSObject

+@property (class, readonly) NSCache *openIdConfigurationCache;
+
 + (BOOL)isADFSInstance:(NSString *)endpoint;
 + (BOOL)isADFSInstanceURL:(NSURL *)endpointUrl;


This seem very duplicative of one another. do we really need these two methods?

One accepting NSURL should be enough here.

These methods weren't added in this pr, I suggest to refactor them during our MSIDAuthority refactoring.

unpluggedk · 2018-05-14T21:56:05Z

IdentityCore/src/validation/MSIDAuthority.m

@@ -137,4 +197,145 @@ + (NSURL *)cacheUrlForAuthority:(NSURL *)authority
    return [NSURL URLWithString:[NSString stringWithFormat:@"https://%@/%@", [authority msidHostWithPortIfNecessary], tenantId]];
 }

+ (void)discoverAuthority:(NSURL *)authority


again, discovery is not what we are solely doing here.

unpluggedk · 2018-05-14T22:04:06Z

IdentityCore/src/validation/MSIDAuthority.m

+    }];
+}
+
+ (NSURL *)normalizeAuthority:(NSURL *)authority


as mentioned before, having this authority as a string would be better for our use case, no?

unpluggedk · 2018-05-14T22:06:56Z

IdentityCore/src/validation/MSIDAuthority.m

+        }
+        return NO;
+    }
+


also check against login.windows.net, which is not deprecated. Advise to use login.microsoftonline.com instead

unpluggedk · 2018-05-14T22:09:06Z

IdentityCore/src/validation/MSIDAuthority.m

+        return NO;
+    }
+
+    if (authority.pathComponents.count < 2)


i agree with possible need for a specific aad check.
However, for check is for all authorities afaik. b2c will have "tfp" as a component, adfs => "adfs" and aad will have it's value.

unpluggedk · 2018-05-14T22:10:03Z

IdentityCore/src/validation/MSIDAuthority.m

+    {
+        if (error)
+        {
+            *error = MSIDCreateError(MSIDErrorDomain, MSIDErrorInternal, @"authority must specify a tenant or common.", nil, nil, nil, context.correlationId, nil);


MSAL also says the same, but we might want to rename this as tenant/common is pretty much AAD only.

unpluggedk · 2018-05-14T22:19:17Z

IdentityCore/src/validation/MSIDB2CAuthorityResolver.m

+                  context:(id<MSIDRequestContext>)context
+          completionBlock:(MSIDAuthorityInfoBlock)completionBlock
+{
+    if (validate && ![MSIDAuthority isKnownHost:authority])


That is correct. Not supported.

…oft-authentication-library-common-for-objc into sedemche/authorityAliases

oldalton · 2018-08-31T21:25:07Z

IdentityCore/IdentityCore.xcodeproj/project.pbxproj

@@ -3518,6 +3724,9 @@
 			isa = XCBuildConfiguration;
 			baseConfigurationReference = D6CF4E961FC3626A00CD70C5 /* identitycore__tests__ios.xcconfig */;
 			buildSettings = {
+				CLANG_ENABLE_MODULES = YES;
+				LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks";
+				SWIFT_VERSION = 3.0;


why did we add this?

oldalton · 2018-08-31T21:25:40Z

IdentityCore/IdentityCore.xcodeproj/project.pbxproj

 				GCC_OPTIMIZATION_LEVEL = 0;
+				LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/../Frameworks @loader_path/../Frameworks";
+				SWIFT_OPTIMIZATION_LEVEL = "-Onone";
+				SWIFT_VERSION = 3.0;


Why do we use swift 3 :)?

oldalton · 2018-08-31T21:28:11Z

IdentityCore/src/network/MSIDAADEndpointProviding.h

+#import <Foundation/Foundation.h>
+#import "MSIDADFSType.h"
+
+@protocol MSIDAADEndpointProviding <NSObject>


nit: endpoint providing should apply also to basic Oauth2 flow, not only to AAD (e.g. we should be able to find an authorize endpoint or openid config for Google). We can take this as a separate issue though.

Added comment about it in #211

oldalton · 2018-08-31T21:29:52Z

IdentityCore/src/network/MSIDURLSessionManager.h

+@property (class, nonnull) MSIDURLSessionManager *defaultManager;
+@property (nonatomic, readonly, nonnull) NSURLSessionConfiguration *configuration;
+@property (nonatomic, readonly, nonnull) NSURLSession *session;
+@property (nonatomic, readonly, nullable) MSIDURLSessionDelegate *delegate;


nit: Why do we need to save delegate outside of passing it to NSURLSession?

you are right, removed it.

oldalton · 2018-08-31T21:32:03Z

IdentityCore/src/network/request/MSIDAADAuthorityMetadataResponseSerializer.m

+    NSError *jsonError;
+    NSDictionary *jsonObject = [super responseObjectForResponse:httpResponse data:data context:context error:&jsonError];
+
+    if (jsonError)


I think it would be more accurate to check if (!jsonObject) here and return early if it's nil.

oldalton · 2018-08-31T21:34:49Z

IdentityCore/src/network/request_serializer/MSIDUrlRequestSerializer.m

+        NSURLComponents *urlComponents = [NSURLComponents componentsWithURL:mutableRequest.URL resolvingAgainstBaseURL:NO];
+        NSMutableDictionary *urlParameters = [[mutableRequest.URL msidQueryParameters] mutableCopy] ?: [NSMutableDictionary new];
+        [urlParameters addEntriesFromDictionary:parameters];
+        urlComponents.percentEncodedQuery = [urlParameters msidURLFormEncode];


Thanks for fixing this!

oldalton · 2018-08-31T21:36:03Z

IdentityCore/src/util/NSDictionary+MSIDExtensions.m

+           ofField:(NSString *)field
+           context:(id <MSIDRequestContext>)context
+         errorCode:(NSInteger)errorCode
+             error:(NSError **)error;


nit: remove ;

oldalton · 2018-08-31T21:36:48Z

IdentityCore/src/util/NSDictionary+MSIDExtensions.m

+                                     nil, nil, context.correlationId, nil);
+        }
+
+        MSID_LOG_ERROR_PII(nil, @"%@", message);


I don't think field name needs to be PII

oldalton · 2018-08-31T21:37:40Z

IdentityCore/src/validation/MSIDAdfsAuthorityResolver.m

+         __auto_type drsCloudRequest = [[MSIDDRSDiscoveryRequest alloc] initWithDomain:domain adfsType:MSIDADFSTypeCloud context:context];
+         [drsCloudRequest sendWithBlock:^(id response, NSError *error)
+          {
+              if (completionBlock) completionBlock(response, error);


Thanks for fixing this

oldalton · 2018-08-31T21:39:53Z

IdentityCore/tests/MSIDDictionaryExtensionsTests.m

+                                      @"key3": @"value3"};
+
+    NSError *error;
+    BOOL result = [inputDictionary assertType:NSString.class ofField:@"key1" context:nil errorCode:1 error:&error];


nit: please also check that correct error was returned.

…ent in headers).

antrix1989 added 6 commits May 3, 2018 19:45

Move authority validation to common core.

334d9b1

Use serial queue for aad validation requests.

66e060f

Support v1 & v2 AAD api endpoints.

9efe74a

Add ADFS authority cache.

6103efe

Add openid metadata cache.

718d5f5

Fix unit tests.

88584ce

antrix1989 requested review from unpluggedk, jasoncoolmax and oldalton May 5, 2018 01:39

Merge branch 'dev' into sedemche/authorityAliases

8cd8321

antrix1989 changed the base branch from sedemche/network_request to dev May 8, 2018 21:39

antrix1989 added 7 commits May 9, 2018 18:51

Cover MSIDAuthority with unit tests. Fix openId configuration loading…

28deb6d

… & parsing.

Cover B2C authority validation with tests.

3fa5855

Cover AAD authority validation with tests.

10c0e5a

Cover ADFS authority validation with tests.

8d1b88c

Don't normalize authority on discovery.

8964c86

Test AAD api version.

564a53a

Remove 'authority' parameter from auth discovery block.

c4920d6

oldalton previously requested changes May 14, 2018

View reviewed changes

unpluggedk reviewed May 14, 2018

View reviewed changes

antrix1989 added 7 commits May 14, 2018 13:33

Verify taht PassiveAuthEndpoint is a string.

1b9bdb6

Fix parentheses position.

45193a7

Move open id configuration suffix to constants.

eaca231

Remove commented code.

01853dc

Rename authority metadata request.

e61f1f3

Fix alignment.

6dfb0cb

Code style fixes.

1f6ecc1

unpluggedk previously requested changes May 14, 2018

View reviewed changes

antrix1989 added 2 commits May 14, 2018 15:37

Add nullability annotation to requests.

95e35eb

Check for deprecated authority.

2904d21

antrix1989 added 2 commits August 28, 2018 16:35

Fix tests and code after merge conflict.

c087868

Merge branch 'sedemche/authorityAliases' of github.com:AzureAD/micros…

4ddf021

…oft-authentication-library-common-for-objc into sedemche/authorityAliases

unpluggedk mentioned this pull request Aug 29, 2018

Webview integration AzureAD/azure-activedirectory-library-for-objc#1262

Merged

antrix1989 added 17 commits August 29, 2018 07:21

Use unmutable dictionary.

582c07b

Small code style fixes.

4b57b29

Put context in MSIDDRSDiscoveryRequest initializer.

851ac1b

Fix realm validation.

eb4b053

Support windows.net authority.

9e48c84

Validate json response of drs discovery.

c401073

Add MSIDErrorAuthorityValidationWebFinger error.

55a04ff

Document responseObjectForResponse method.

17efef8

Modify trusted hosts.

b7a13c1

Invoke completition block when on prem & cloud drs discrovery failed.

fd756c3

Log host of challendge's protection space.

f415923

Check for nil opendID configuration url.

04e8c61

Verify that openIdConfigurationEndpoint is not nil and NSString.

8c6a2c6

Provide context in initializer.

c5d365a

Use msidURLFormEncode instead of NSURLComponents encoding.

1c16743

Merge branch 'dev' into sedemche/authorityAliases

ed78126

Cover error description parsing logic with tests.

ae4c191

oldalton approved these changes Aug 31, 2018

View reviewed changes

antrix1989 added 8 commits August 31, 2018 16:56

Don't store seesion delegate in ivar.

3a28811

Return early if json is nil.

f5c9d91

Remove semicolon.

86e859e

Log as not pii.

bbc6a7c

Check error details in tests.

917c0c3

Validate MSIDOpenIdConfigurationInfoRespons json.

37a1276

Remove swift settings from proj file.

69233e9

Remove x-client-ver from query parameters of network request (it is s…

0b676f1

…ent in headers).

antrix1989 merged commit 81d2269 into dev Sep 1, 2018


		#import <Foundation/Foundation.h>

		@interface MSIDAuthorityCacheRecord : NSObject


		@interface MSIDOpenIdProviderMetadata : NSObject

		@property (nonatomic) NSURL *authorizationEndpoint;


		#import <Foundation/Foundation.h>

		@interface MSIDConfiguration : NSObject


		@end

		@interface MSIDAADGetAuthorityMetadataRequest : MSIDHttpRequest

Move authority migration to common core #130

Move authority migration to common core #130

Conversation

antrix1989 commented May 5, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unpluggedk 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

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

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

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