Documentation for adding credentials #302

Closed
pokstad opened this Issue Jul 17, 2016 · 2 comments

Comments

Projects
None yet
3 participants
@pokstad

pokstad commented Jul 17, 2016

The documentation states that passwords should be encoded in NSURLs:

// username/password can be Cloudant API keys
NSString *s = @"https://username:password@username.cloudant.com/my_database";

This is not possible with complex passwords that contain special delimiter tokens, such as the @ (at) symbol which is used to end the password or : (colon) which is used to end the username or / (backslash) which is used to end the protocol and start the username. NSURL does not have any methods to set the username and password with appropriate encoding to avoid this issue. Ultimately, these credentials will be added as basic authentication header in base64 encoding. Instead of adding them to the NSURL, they should be added directly to the headers to avoid parsing issues. This can be done with the HTTP interceptors:

func basicAuthHeaderInBase64() -> String? {
    if let username = fetchUsername(),
       let password = fetchPassword() {
        let userPasswordString = "\(username):\(password)"
        let userPasswordData = userPasswordString.dataUsingEncoding(NSUTF8StringEncoding)
        let base64EncodedCredential = userPasswordData!.base64EncodedStringWithOptions(
            .Encoding64CharacterLineLength)
        return "Basic \(base64EncodedCredential)"
    }
    return nil
}

func interceptRequestInContext(context: CDTHTTPInterceptorContext) -> CDTHTTPInterceptorContext {
    if let authString = basicAuthHeaderInBase64() {
        context.request.setValue(
            authString,
            forHTTPHeaderField: "Authorization")
    } else {
        // anonymous access requires us to remove authorization header
        context.request.setValue(nil, forHTTPHeaderField: "Authorization")
    }
    return context
}

Ideally, the NSURLSessionConfiguration object should be customized via delegate method to add basic auth:

var credential: NSURLCredential {
       return cred = NSURLCredential(
                user: self.username,
                password: self.password,
                persistence: .ForSession)
}    

var protectionSpace: NSURLProtectionSpace {
    return NSURLProtectionSpace(
            host: self.url.host,
            port: self.port,
            protocol: self.url.scheme,
            realm: nil,
            authenticationMethod: NSURLAuthenticationMethodHTTPBasic)
}

@objc func customiseNSURLSessionConfiguration(config: NSURLSessionConfiguration) {
    if let c = credential, p = protectionSpace {
        config.URLCredentialStorage?.setCredential(c, forProtectionSpace: p)
    }
}

While testing this method I found that two different replications from two different users initiated in the same protection space will not run as expected. This is probably because the URLCredentialStorage object is a shared object between all URLSessions (not verified). Each time a replication is run, it will pick a default credential from the protection space and NOT the last credential set. In order to avoid authentication errors, a work around is to remove ALL credentials for a protection space before adding the credentials desired (relevant stack overflow question). However, this ends up modifying the keychain which is not ideal if you are depending on the keychain to store your user credentials.

Another solution is to set the default credential for a protection space. However, this will not work if you want to be able to use anonymous credentials.

A more ideal solution is to use the NSURLSession delegate to handle authentication challenges. This way, based on your app's specific logic you can choose which credential to retrieve from the credential store and use that instead. Currently, I cannot find anywhere in the library that exposes the NSURLSession delegate property for this customization.

@rhyshort rhyshort added the bug label Jul 18, 2016

@rhyshort

This comment has been minimized.

Show comment
Hide comment
@rhyshort

rhyshort Jul 18, 2016

Member

It sounds like we have a couple of issues here. The way we accept credentials into the replicator isn't perfect, and since we strip them from the URL to use with _session endpoint with CouchDB, we should probably expose this as a username and password properties, with the URL stripping as a way of maintaining compatibility.

I don't think exposing the delegate challenge is the right thing to do, since hiding it away means we can switch out the implementation without a breaking change in the event it is needed, we should do some thinking on it and see what we come up with.

I think there are a few things need to here:

  • Have a better API for providing usernames and passwords, I think this should be via a property.
  • Change the NSURLSessionConfiguration so it doesn't share storage and cache etc.
  • Ensure the url-form-encoding is correct in the CDTSessionCookieInterceptor.
Member

rhyshort commented Jul 18, 2016

It sounds like we have a couple of issues here. The way we accept credentials into the replicator isn't perfect, and since we strip them from the URL to use with _session endpoint with CouchDB, we should probably expose this as a username and password properties, with the URL stripping as a way of maintaining compatibility.

I don't think exposing the delegate challenge is the right thing to do, since hiding it away means we can switch out the implementation without a breaking change in the event it is needed, we should do some thinking on it and see what we come up with.

I think there are a few things need to here:

  • Have a better API for providing usernames and passwords, I think this should be via a property.
  • Change the NSURLSessionConfiguration so it doesn't share storage and cache etc.
  • Ensure the url-form-encoding is correct in the CDTSessionCookieInterceptor.
@tomblench

This comment has been minimized.

Show comment
Hide comment
@tomblench

tomblench Jul 19, 2016

Contributor

Firstly, : can't be used in usernames for basic auth because of the way it's encoded over the network:

userid      = *<TEXT excluding ":">

(https://www.ietf.org/rfc/rfc2617.txt)

But of course you should be able to use any other characters and tell our library that you want to use these.

Maybe we should do what some of our other libraries do and allow for username/password in URL or as extra configuration, with the latter over-riding the former. I still think it's useful to have these in the URL but I guess we have stored up trouble for ourselves by pretending we can transparently 'upgrade' the user from basic auth to cookie auth.

As a case in point, I can access local couch with username and password both of @@@ using curl http://%40%40%40:%40%40%40@localhost:5984/_all_dbs. But this doesn't seem to work in our library, presumably because cookie auth has no idea about percent encoding.

Contributor

tomblench commented Jul 19, 2016

Firstly, : can't be used in usernames for basic auth because of the way it's encoded over the network:

userid      = *<TEXT excluding ":">

(https://www.ietf.org/rfc/rfc2617.txt)

But of course you should be able to use any other characters and tell our library that you want to use these.

Maybe we should do what some of our other libraries do and allow for username/password in URL or as extra configuration, with the latter over-riding the former. I still think it's useful to have these in the URL but I guess we have stored up trouble for ourselves by pretending we can transparently 'upgrade' the user from basic auth to cookie auth.

As a case in point, I can access local couch with username and password both of @@@ using curl http://%40%40%40:%40%40%40@localhost:5984/_all_dbs. But this doesn't seem to work in our library, presumably because cookie auth has no idea about percent encoding.

@rhyshort rhyshort self-assigned this Jul 25, 2016

rhyshort added a commit that referenced this issue Jul 25, 2016

New credential API.
Added new credential API to CDT*Replication classes. This API provides
new arguments on the replication class methods to provide username and
passwords to the be used when authenticating with the server.

Part of #302

rhyshort added a commit that referenced this issue Jul 25, 2016

New credential API.
Added new credential API to CDT*Replication classes. This API provides
new arguments on the replication class methods to provide username and
passwords to the be used when authenticating with the server.

Part of #302

rhyshort added a commit that referenced this issue Jul 25, 2016

New credential API.
Added new credential API to CDT*Replication classes. This API provides
new arguments on the replication class methods to provide username and
passwords to the be used when authenticating with the server.

Part of #302

rhyshort added a commit that referenced this issue Sep 9, 2016

New credential API.
Added new credential API to CDT*Replication classes. This API provides
new arguments on the replication class methods to provide username and
passwords to the be used when authenticating with the server.

Part of #302

rhyshort added a commit that referenced this issue Sep 15, 2016

New credential API.
Added new credential API to CDT*Replication classes. This API provides
new arguments on the replication class methods to provide username and
passwords to the be used when authenticating with the server.

Part of #302

rhyshort added a commit that referenced this issue Sep 27, 2016

New credential API.
Added new credential API to CDT*Replication classes. This API provides
new arguments on the replication class methods to provide username and
passwords to the be used when authenticating with the server.

Part of #302

@rhyshort rhyshort closed this Oct 5, 2016

@rhyshort rhyshort referenced this issue in cloudant/sync-android Dec 12, 2016

Closed

ReplicatorBuilder API for Passing Creds #440

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment