Skip to content

Basic implementation of URLCredentialStorage #2334

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

Merged
merged 1 commit into from
Jul 17, 2019

Conversation

drodriguez
Copy link
Contributor

A basic implementation of URLCredentialStorage, mainly oriented to
credentials for the session. It is an in-memory storage and ignores any
usage of permantent or synchronizable persistency. This should cover
many usage cases, and should allow to built on top of it.

Includes tests.

Known differences with macOS:

I tried to run the set of test against macOS (10.14.5) and found a couple of weird results (I run the tests in a playground, so that might cause interesting behaviours).

  • .permanent credentials are only stored in-memory (so they are equivalent to .forSession). Each operating system might have their own permanent storage, so each implementation might be different (or not exist at all). This can be tackled after this basic implementation is reviewed, and in a per-OS strategy.
  • Seems that URLCredentialStorage() in macOS will not return a storage that can store credentials (not even .forSession ones). In the tests I decided to use URLCredentialStorage.shared since it will work on macOS, but makes the tests harder to read, because you have to implicitely clean.
  • Seems that setting a credential for a protection space, even if it is not through setDefaultCredential(…), sets it as default.
  • I could not get the change notifications to happen in macOS. I added the notifications nonetheless where I think they should happen.
  • I could not add a .synchronizable credential in macOS. I imagine the playground I was using might not have the right permissions for the iCloud Keychain. However, in the implementation, the .synchronizable credentials are added to the in-memory storage, and are removed only when providing the special key.

@millenomi
Copy link
Contributor

@swift-ci please test linux

@millenomi
Copy link
Contributor

Yeah, you need to have an entitled client (i.e., an App Store or Developer ID app) to access iCloud Keychain.

I'd rather that we don't add .synchronizable credentials. When we accept doing a thing, we may risk introducing people who rely on it, or port existing code that relies on it, and make assumptions about the way it works. Since no process is entitled on Linux, we should just fail to save the way macOS does.

@millenomi
Copy link
Contributor

I'm fine with everything else! Unless someone wants to contribute persistency to actual secure storage (say, the GNOME keyring).

Fixes https://bugs.swift.org/browse/SR-10375 (which we're repurposing for both the storage and the credentials)

@drodriguez
Copy link
Contributor Author

OK. I will do some changes to make very obvious that .synchronizable credentials are not supported. It is not clear if I should do the same with .permanent, though.

Another difference I forgot in the initial summary: I have not implemented the methods that receive URLSessionTask. The documentation is not very clear, but from some clues they might be intended for subclasses. I will provide simple implementations for the base class.

A basic implementation of URLCredentialStorage, mainly oriented to
credentials for the session. It is an in-memory storage and ignores any
usage of permantent or synchronizable persistency. This should cover
many usage cases, and should allow to built on top of it.

Includes tests.
@drodriguez drodriguez force-pushed the url-credential-storage branch from 66a8372 to ddeab1d Compare June 12, 2019 18:09
@drodriguez
Copy link
Contributor Author

@swift-ci please test linux

@millenomi: I also can keep working in #2312, if you want, I would love to have NSCache more or less working.

@millenomi
Copy link
Contributor

@swift-ci please test linux

@millenomi
Copy link
Contributor

I've been working on a parallel implementation that also has disk support.

@drodriguez
Copy link
Contributor Author

@swift-ci please test Linux platform

@millenomi millenomi merged commit 9e505a9 into swiftlang:master Jul 17, 2019
@drodriguez drodriguez deleted the url-credential-storage branch July 22, 2019 18:05
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.

2 participants