Add a GTCred wrapper. #254

tiennou · 2013-09-10T20:21:37Z

This adds support to libgit2 cred.h API in advance of #224 and #252 (and push when the time comes). I'll obviously massage #224 to use it, and @isaac if you have the will (and time) to do the same with #252, that's cool, otherwise it can wait for another PR.

It feels more or less ready except for :

This isn't used anywhere, as we have no fetch/push support yet, and Add options argument to GTRepository cloneFromURL #252 "prevents" me from switching clone to it.
I have absolutely no idea how to test it because it depends on a working authenticated transport.
There's a big PARTIALIMPL in one of the class methods, because I was happily wrapping stuff around, but when the time came for me to look up LIBSSH2_USERAUTH_PUBLICKEY_SIGN_FUNC in libssh2, I just, like, shivered in fear when I saw the LIBSSH2_SESSION parameter. That parameter means that if libgit2 ever asks for a GIT_CREDTYPE_SSH_PUBLICKEY auth type, we have to traverse layers of C-code to get to the SSH layer and back again. As my experience with the libgit2 code is minimal, it will take some time for me to see if there's a general solution.

tiennou · 2013-09-10T20:26:54Z

Heh, looks like it's in fact libgit2 that's gently pushing its LIBSSH2_SESSION so that we can do... something with it. So the third point above can be replaced by "requires libssh2 to be around" ;-).

Readmore · 2013-09-10T21:46:03Z

I'm looking for a way to clone a git repo with authentication ( ssh or https would work ). I'm assuming that is your goal here. How are you planning to implement the authenticated call? Will you be able to add username and password params to the clone call?

tiennou · 2013-09-10T23:24:27Z

@Readmore Normally, the clone auth support will be added when this one and #252 are merged in.

jspahrsummers · 2013-09-11T19:01:15Z

Classes/GTCred.h

+// A typedef block for the various methods that require authentication
+typedef GTCred *(^GTCredBlock)(GTCredentialType allowedTypes, NSString *URL, NSString *username);
+
+@interface GTCred : NSObject


How about GTCredential? That's more consistent with NSURLCredential and Cocoa's verbosity.

Agreed, will change it to GTCredential. I just got thrown of by libgit2's name for it ;-).

jspahrsummers · 2013-09-11T19:06:23Z

What's the issue with using libssh? It can be installed via Homebrew, and #250 is adding it for iOS.

tiennou · 2013-09-11T21:38:16Z

I'm sure I'm missing something, but I added "support" for libssh2 in #200, which was reverted because it broke CI. True, I never asked if the breakage was due to the iOS build or the OS X. But if libssh2 is now expected to be a "hard" dependency, then there's no issue anymore, and I just need to find some time to understand the 3rd auth type and why it's passing LIBSSH2_SESSION around (there's a reason for that, but right now it's out of my reach).

Conflicts: External/Configuration External/libgit2 External/openssl

jspahrsummers · 2013-09-16T04:41:53Z

@tiennou I think I've fixed all the libssh2 build issues on master, if you wanna merge it in and give that a shot.

tiennou · 2013-09-16T10:43:25Z

🍯

I added a GTCredentialProvider, in fact I like the idea ;-). I just made the default "implementation" wrap the old block API I had (for easy I-don't-want-to-subclass uses). I won't say more, I expect the documentation to do it for me ;-). Feel free to point out discrepancies, missing/unclear information.

Also I asked on SO about the GTCredentialTypeSSHPublicKey auth type, and it seems like we're supposed to sign data with it. I propose to make the corresponding GTCredential method private until this can be implemented...

This takes paths to key files, not actual key data.

That `GIT_SSH` define is needed in the project to make both `LIBSSH_SESSION` and the correct typedef for `git_cred_sign_callback` visible. I do think it's a quirk of libgit2 though...

tiennou · 2013-09-16T23:39:55Z

This more or less depends on libgit2/libgit2#1851 and libgit2/libgit2#1853 now (less on 1851, more on 1853 ;-)). I also made sufficient (I hope) research to implement the 3rd method, so it's now documented. I've built a basic clone app for that, so if there's interest in that, maybe I can put in in some Examples directory (I would have made a test, but SSH keys, passwords and test files don't go along too well).

jspahrsummers · 2013-09-17T05:04:59Z

Classes/GTCredential+Private.h

+	__unsafe_unretained GTCredentialProvider *credProvider;
+} GTCredentialAcquireCallbackInfo;
+
+int GTCredentialAcquireCallback(git_cred **cred, const char *url, const char *username_from_url, unsigned int allowed_types, void *payload);


This can be done with a method on GTCredentialProvider now.

Huh ? That's the C trampoline that calls GTCredentialProvider, so no, I can't do that. You're supposed to pass it to libgit2 APIs that take a git_cred_aquire_cb (as in the fetch example I've given above).

Ah, sorry, you're right.

jspahrsummers · 2013-09-25T20:56:35Z

Classes/GTCredential.m

+	NSParameterAssert(privateKeyURL != nil);
+	NSString *publicKeyPath = publicKeyURL.filePathURL.path;
+	NSString *privateKeyPath = privateKeyURL.filePathURL.path;
+	NSAssert(privateKeyPath != nil, @"Invalid file URL passed: %@", privateKeyURL);


This should probably be an NSError, not an assertion (e.g., in case the path/URL is user-provided).

I see two problems ;-). Maybe it's better to assert because it forces the developer to validate the path before blindly giving it to us (it's gonna get used while the operation is in progress, which means a network operation that is doomed to fail if you don't check before. The other problem is that our current NSError machinery isn't really adapted to non-libgit2 errors (yet ;-)).
Also, this assert merely checks that the user isn't passing anything else than a file:// URL in there (I'm asserting for nil just above, but we require a local path in there).

About the "yet" part, would you prefer me to rename the category and make it our own GTError ? I don't plan on having any instance methods in there, just the current class methods less the git_ prefix. It's just that I find the category to be a bother, and git_errorForGitError: sounds a little too much git ;-).

This is fine for now, I guess. Your points about validation are good.

jspahrsummers · 2013-09-25T21:07:32Z

🍊

Conflicts: Classes/Categories/NSError+Git.m

Else test building fails on the <libssh2.h> include.

tiennou · 2013-09-30T17:54:52Z

📗

jspahrsummers · 2013-09-30T21:08:29Z

Classes/GTCredential.m

+typedef GTCredential *(^GTCredentialProviderBlock)(GTCredentialType allowedTypes, NSString *URL, NSString *userName);
+
+@interface GTCredentialProvider ()
+@property (nonatomic, readonly) GTCredentialProviderBlock credBlock;


This should still be copy to indicate its implemented semantics.

jspahrsummers · 2013-09-30T21:09:45Z

📘

tiennou · 2013-10-01T07:40:37Z

🚗

jspahrsummers · 2013-10-01T18:36:22Z

Add a GTCred wrapper.

Add a GTCred wrapper.

c9d19c4

ghost assigned jspahrsummers Sep 11, 2013

jspahrsummers reviewed Sep 11, 2013
View reviewed changes

isaac mentioned this pull request Sep 13, 2013

Add options argument to GTRepository cloneFromURL #252

Merged

tiennou added 2 commits September 15, 2013 12:07

Merge remote-tracking branch 'origin/master' into git-cred

4767309

Conflicts: External/Configuration External/libgit2 External/openssl

Rename GTCred => GTCredential.

7d58522

ghost assigned jspahrsummers Sep 16, 2013

tiennou added 2 commits September 16, 2013 11:56

Add GTCredentialProvider class + documentation.

e8c3dde

Merge branch 'master' into git-cred

c472898

One line userInfo creation.

cff6753

tiennou mentioned this pull request Sep 16, 2013

Provide libssh2 error message libgit2/libgit2#1851

Merged

tiennou added 4 commits September 16, 2013 23:29

Update as per libgit2 documentation.

23b4253

This takes paths to key files, not actual key data.

Small documentation fixes.

2823345

Implement the 3rd method.

6cc9a66

That `GIT_SSH` define is needed in the project to make both `LIBSSH_SESSION` and the correct typedef for `git_cred_sign_callback` visible. I do think it's a quirk of libgit2 though...

Allow passing a GTCredentialProvider when cloning.

64c0c6d

Merge branch 'master' into git-cred

05c21a1

jspahrsummers reviewed Sep 17, 2013
View reviewed changes

jspahrsummers reviewed Sep 25, 2013
View reviewed changes

tiennou added 8 commits September 25, 2013 23:45

Merge branch 'master' into git-cred

b66ba1a

Conflicts: Classes/Categories/NSError+Git.m

Move -DGIT_SSH at the project level.

f21c87a

Provide the paths to the public and private keys in the error message.

4832625

No need for formatting.

cc2670f

Style: explicit nil comparison.

29f132b

Also move that one to the project level

3fb9a46

Else test building fails on the <libssh2.h> include.

Use the typedef-ed block in that signature.

0f9b7c9

Make the provider's block ivar nonatomic, readonly.

9d69126

jspahrsummers reviewed Sep 30, 2013
View reviewed changes

tiennou added 2 commits October 1, 2013 09:37

Copy that.

e29d347

Use a C string instead of NSString.

32c00c2

jspahrsummers added a commit that referenced this pull request Oct 1, 2013

Merge pull request #254 from libgit2/git-cred

10104b2

Add a GTCred wrapper.

jspahrsummers merged commit 10104b2 into master Oct 1, 2013

jspahrsummers deleted the git-cred branch October 1, 2013 18:36

phatblat pushed a commit to phatblat/objective-git that referenced this pull request Sep 13, 2014

Merge pull request libgit2#254 from libgit2/git-cred

ebdf2b3

Add a GTCred wrapper.

jspahrsummers removed their assignment May 22, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a GTCred wrapper. #254

Add a GTCred wrapper. #254

tiennou commented Sep 10, 2013

tiennou commented Sep 10, 2013

Readmore commented Sep 10, 2013

tiennou commented Sep 10, 2013

jspahrsummers Sep 11, 2013

tiennou Sep 11, 2013

jspahrsummers commented Sep 11, 2013

tiennou commented Sep 11, 2013

jspahrsummers commented Sep 16, 2013

tiennou commented Sep 16, 2013

tiennou commented Sep 16, 2013

jspahrsummers Sep 17, 2013

tiennou Sep 17, 2013

jspahrsummers Sep 17, 2013

jspahrsummers Sep 25, 2013

tiennou Sep 25, 2013

tiennou Sep 25, 2013

jspahrsummers Sep 26, 2013

jspahrsummers commented Sep 25, 2013

tiennou commented Sep 30, 2013

jspahrsummers Sep 30, 2013

jspahrsummers commented Sep 30, 2013

tiennou commented Oct 1, 2013

jspahrsummers commented Oct 1, 2013

Add a GTCred wrapper. #254

Add a GTCred wrapper. #254

Conversation

tiennou commented Sep 10, 2013

tiennou commented Sep 10, 2013

Readmore commented Sep 10, 2013

tiennou commented Sep 10, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jspahrsummers commented Sep 11, 2013

tiennou commented Sep 11, 2013

jspahrsummers commented Sep 16, 2013

tiennou commented Sep 16, 2013

tiennou commented Sep 16, 2013

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

jspahrsummers commented Sep 25, 2013

tiennou commented Sep 30, 2013

Choose a reason for hiding this comment

jspahrsummers commented Sep 30, 2013

tiennou commented Oct 1, 2013

jspahrsummers commented Oct 1, 2013