-
Notifications
You must be signed in to change notification settings - Fork 281
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
Clone fetch auth #203
Conversation
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use BOOL
.
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 |
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... |
static NSMutableDictionary *credentials; | ||
|
||
+ (void)cacheCredentialsForCallBack:(NSString*)url username:(NSString*)username password:(NSString*)password { | ||
if (!credentials) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
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; |
There was a problem hiding this comment.
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
}];
There was a problem hiding this comment.
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
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 Please keep in mind the user/pass API won't fit with SSH so it should be general enough. |
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_EXTERN(int) git_cred_ssh_keyfile_passphrase_new( So we can go this way: When a developer wants to implement authentication: new block handler is created with three options:
For the beginning I would leave git_cred type as is (without creating corresponding GTCred class) |
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 ;-). |
ok |
@andrewromanenco Any update on this ? I'm more-or-less waiting on the auth part for push support... |
Hi will try to finish in next couple of days... |
Then forget about it ;-). I'm actually working on it. |
ok |
@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. |
Extension:
These extensions were developed for this app:
https://github.com/AndrewRomanenco/git.ios