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.

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

[TIMOB-19154]: iOS9 Deprecate NSURLConnection and replace with NSURLSession API #46

Merged
merged 11 commits into from Feb 22, 2018

Conversation

vijaysingh-axway
Copy link
Contributor

@hansemannn
Copy link
Contributor

@vijaysingh-axway This PR does not include the two-way TLS-authentication so far. See #49 for how it's generally handled.

Copy link
Contributor

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

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

This PR is missing some newer functionalities added over the last months.

Copy link
Contributor

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

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

Left a few review comments, mostly formatting for now. Will do another review-round this week.

@@ -232,6 +249,192 @@ -(BOOL)willHandleChallenge:(NSURLAuthenticationChallenge *)challenge forConnecti

#pragma mark NSURLConnectionDelegate methods

-(void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didReceiveChallenge:(NSURLAuthenticationChallenge *)challenge completionHandler:(void (^)(NSURLSessionAuthChallengeDisposition, NSURLCredential * _Nullable))completionHandler
Copy link
Contributor

Choose a reason for hiding this comment

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

Space

@@ -215,6 +215,23 @@ - (ClientCertificate *)clientCertificateForHost:(NSString *)host {
// Return FALSE unless the NSURLAuthenticationChallenge is for TLS trust
// validation (aka NSURLAuthenticationMethodServerTrust) and this security
// manager was configured to handle the current url.

-(BOOL)willHandleChallenge:(NSURLAuthenticationChallenge *)challenge forSession:(NSURLSession *)session {
Copy link
Contributor

Choose a reason for hiding this comment

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

Space



SecTrustRef serverTrust = challenge.protectionSpace.serverTrust;
if(serverTrust == nil) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Space

// return *non-success*. Unusual conditions include an
// expired certifcate or self signed certifcate.
OSStatus status = SecTrustEvaluate(serverTrust, NULL);
if(status != errSecSuccess) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Space


// Obtain the server's X509 certificate and public key.
SecCertificateRef serverCertificate = SecTrustGetCertificateAtIndex(serverTrust, pinnedPublicKey.trustChainIndex);
if(serverCertificate == nil) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Space

// Compare the public keys. If they match, then the server is
// authenticated.
BOOL publicKeysAreEqual = [pinnedPublicKey isEqualToPublicKey:serverPublicKey];
if(!publicKeysAreEqual) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Space

DebugLog(@"%s returns %@, challenge = %@, session = %@ URL = %@", __PRETTY_FUNCTION__, NSStringFromBOOL(result), challenge, session, challenge.protectionSpace.host);
return result;
}

-(BOOL)willHandleChallenge:(NSURLAuthenticationChallenge *)challenge forConnection:(NSURLConnection *)connection {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the old NSURLConnection references and bump the module version to the next major.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not remove NSURLConnection in this release. If someone is using this module(this version) with SDK < 7.1.0, in that case it will create problem. Reason is, before 7.1.0 APSHTTPClient will use NSURLConnection apis from this module. Rather we should plan to remove NSURLConnection from this module with SDK 8.0.0. Till then most of developer will have moved to SDK 7.1.0+ . I'll create a ticket for same.
And this version should be released with/before, SDK 7.1.0. With SDK 7.1.0 and above , only this version of this module will work.

Copy link
Contributor

@hansemannn hansemannn Jan 3, 2018

Choose a reason for hiding this comment

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

That's a good reason. I thought it's not compatible anymore, but thats the other way around right? If devs update to 7.1.0, they also need to update to the latest https module. Talking about that, please bump the version to the next feature version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes with 7.1.0, latest https is mandatory. Updated version.

@hansemannn
Copy link
Contributor

@vijaysingh-axway Please pull from master and use 2.3.0 as the version as 2.2.0 was used to support two-way-SSL support (see here).

@hansemannn
Copy link
Contributor

Thanks! I guess QE will also need a packaged version of this module.

@ewieberappc
Copy link
Contributor

FR Passed. Tested using all of the sample and referenced tickets in the issue ticket as well as the module examples and http test suite

@ewieberappc ewieberappc merged commit 3a79a5b into tidev:master Feb 22, 2018
@hansemannn
Copy link
Contributor

hansemannn commented Mar 8, 2018

@vijaysingh-axway Please create a new release version for this change. I am about to handle #56 thats 2.4.0, so 2.3.0 should be listed before. Also, please check with Eric, so the module is uploaded to dashboard once 7.1.0. I have a bit of a bad feeling that it might confuse people, but we need to put the minimum SDK version to 7.1.0 as well to use the NSURLSession API. That can be done via dashboard (<= v2.2.0 for SDK < 7.1.0 and >= v2.3.0 for SDK >= 7.1.0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants