Skip to content

Clone fetch auth #203

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

Closed

Conversation

andrewromanenco
Copy link

Extension:

  • Password based authentication when clone http(s) repository
  • Fetch changes from remote
  • Fetch changes from remote with password based authentication

These extensions were developed for this app:
https://github.com/AndrewRomanenco/git.ios

// error(out) - If not NULL, set to any error that occurs.
//
// Returns true if fetched.
- (bool)fetchFromRemote:(NSString*)name transferProgressBlock:(void (^)(const git_transfer_progress *))transferProgressBlock error:(NSError **)error;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use BOOL.

@tiennou
Copy link
Contributor

tiennou commented Jul 8, 2013

Since I was working (a little) on this, I was wondering how to handle credentials gracefully (given that SSH adds keys and stuff instead of user/password). I was thinking that having a delegate-based call would be nice (because it would allow us to drop the arguments to the -clone... and - fetch... methods (and -push... when we come around to support it). Also, maybe the low-level fetch machinery would better sit in GTRemote directly (and have GTRepository have a -fetchAllRemotes...call) ?

@andrewromanenco
Copy link
Author

Etienne,

Both notes are good.

I'll move fetch code to GTRemote - this makes way more sense.

Will try to implement delegate based approach - which should not be a problem for password based authentication; will check for ssh...

@ghost ghost assigned joshaber Jul 8, 2013
static NSMutableDictionary *credentials;

+ (void)cacheCredentialsForCallBack:(NSString*)url username:(NSString*)username password:(NSString*)password {
if (!credentials) {
Copy link
Member

Choose a reason for hiding this comment

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

We've been working hard to make objective-git thread safe and unfortunately this is not. Could you fix that up please?

Copy link
Author

Choose a reason for hiding this comment

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

actually, my implementation is also not thread safe :(
as for now - I am using it in ios app which is single threaded by design (I mean app allows only one git operation at a time)

after this particular commit will be done, I'll give a try...

@joshaber
Copy link
Member

joshaber commented Jul 8, 2013

I'll go ahead and stop here as a lot of the notes I made about style apply throughout. You can find a semi-thorough list of conventions at https://github.com/github/objective-c-conventions/blob/master/README.md.

// Cache plain credentials with respect to url authentication callback.
+ (void)cacheCredentialsForCallBack:(NSString*)url username:(NSString*)username password:(NSString*)password;
+ (NSArray *)credentialsForUrl:(NSString*)url;
+ (void)forgetCredentialsForUrl:(NSString*)url;
Copy link
Member

Choose a reason for hiding this comment

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

From how these APIs are used currently, it seems like a block-based API would be more foolproof.

[GTRepository cacheCredentials... withBlock:^{
    // do stuff with them
}];

Copy link
Author

Choose a reason for hiding this comment

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

agree, will check this out

@tiennou
Copy link
Contributor

tiennou commented Jul 9, 2013

I published my branch "for reference" here if you want to take a look (that's obviously unfinished because I needed to go look at libssh2 docs to know what to do with the GIT_CREDTYPE_SSH_PUBLICKEY cred type — that's the point where I stopped working on it actually ;-)).

Please keep in mind the user/pass API won't fit with SSH so it should be general enough.

@andrewromanenco
Copy link
Author

Hi

To authenticate our handler has to return git_cred

My understanding is that authentication is build around these helpers:

GIT_EXTERN(int) git_cred_userpass_plaintext_new(
git_cred **out,
const char *username,
const char *password);

GIT_EXTERN(int) git_cred_ssh_keyfile_passphrase_new(
git_cred **out,
const char *publickey,
const char *privatekey,
const char *passphrase);

So we can go this way:
add authentication block handler to pass to clone/fetch
add two (static?) helper function around plaintext and keyfile

When a developer wants to implement authentication: new block handler is created with three options:

  1. implement new call back
  2. use helper method with username and password
  3. use helper method with keyfiles

For the beginning I would leave git_cred type as is (without creating corresponding GTCred class)

@tiennou
Copy link
Contributor

tiennou commented Jul 10, 2013

Yes, looks like this would work. I'm not sure about adding more helper functions though, because there are many places where those passwords/keys would be found (Keychain seems an obvious place for both, but there's also GitHub tokens in the config files). Let's say the application knows better and move along ;-).

@andrewromanenco
Copy link
Author

ok

@tiennou
Copy link
Contributor

tiennou commented Jul 15, 2013

@andrewromanenco Any update on this ? I'm more-or-less waiting on the auth part for push support...

@andrewromanenco
Copy link
Author

Hi

will try to finish in next couple of days...

@tiennou
Copy link
Contributor

tiennou commented Jul 15, 2013

Then forget about it ;-). I'm actually working on it.

@tiennou tiennou mentioned this pull request Jul 15, 2013
@andrewromanenco
Copy link
Author

ok

@jspahrsummers
Copy link
Contributor

@andrewromanenco Thank you for the pull request! Feel free to comment on any part of #224 if there's anything you think was missed there.

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.

4 participants