Skip to content

Add credentials support - #292

Merged
vmg merged 3 commits into
developmentfrom
arthur/credentials
Dec 9, 2013
Merged

vmg merged 3 commits into
developmentfrom
arthur/credentials

Conversation

@arthurschreiber

Copy link
Copy Markdown
Member

This is a first rough draft for credentials support in Rugged.

It tries to expose a simple as well as flexible API.

For simple use-case (like scripting/automation, where the credentials are most likely known/fixed), it allows passing a Rugged::Credentials object directly in the options hash to Rugged::Repository#clone_at:

repo = Rugged::Repository.clone_at("git@github.com:arthurschreiber/private.git", dir, {
  credentials: Rugged::Credentials::SshKey.new("git", File.expand_path("~/.ssh/id_rsa.pub"), File.expand_path("~/.ssh/id_rsa"), "passphrase")
})

For more complex use cases, like GUIs, it allows passing a proc instead. This proc can then handle the credential lookup:

repo = Rugged::Repository.clone_at("git@github.com:arthurschreiber/private.git", dir, {
  credentials: lambda { |url, username, allowed_types|
    # Dynamically create the correct credentials object, based on the url, username, and/or
    # allowed credential types.
    return Rugged::Credentials::SshKey.new("git", File.expand_path("~/.ssh/id_rsa.pub"), File.expand_path("~/.ssh/id_rsa"), "passphrase")
  }
})

This is not yet completely ready to be merged, I'm mainly looking for feedback on the API.

@arthurschreiber

Copy link
Copy Markdown
Member Author

@vmg Can you take a look? This is basically ready-to-go if you are fine with the API. It even comes with a limited amount of tests based on the travis setup @cmn added to libgit2 some while ago.

Roughly based on the libgit2 online test setup.
Comment thread lib/rugged/credentials.rb Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The comment indicates that the passphrase is optional, but it is a required parameter. It should default to ""

def initialize(username, publickey, privatekey, passphrase="")

However, this seems like too many ordered parameters to me. It is too easy to mix up the arguments ("did the publickey go first, or the privatekey?"). I would prefer hash style arguments, so the class can be initialized like so:

SshKey.new(
  username: "git", 
  public_key: File.expand_path("~/.ssh/id_rsa.pub"), 
  private_key: File.expand_path("~/.ssh/id_rsa"), 
  passphrase: "passphrase"
)

The passphrase being optional, of course. The same would apply to Credentials::Plaintext, for consistency sake.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I agree. A hash would be better suited for initializing the key.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍 That's an easy change.

@dangerp

dangerp commented Dec 5, 2013

Copy link
Copy Markdown

This functionality would also need to be added to Rugged::Remote#connect as well, correct? For instances where you would already have a repo object perhaps the Rugged::Repository object should be initialized with them?

repo = Rugged::Repository.new("path/to/repo.git", credentials: ssh_key_object)
Rugged::Remote.lookup(repo, "origin").connect(:fetch) # uses the SSH credentials from the repo object

@arthurschreiber

Copy link
Copy Markdown
Member Author

@dangerp Setting credentials on a Repository is wrong, as a repository can have multiple remotes that each can require different credentials. Instead, I'll add a way to set the credentials for a remote.

vmg pushed a commit that referenced this pull request Dec 9, 2013
@vmg
vmg merged commit 9e1cc39 into development Dec 9, 2013
@dangerp

dangerp commented Dec 9, 2013

Copy link
Copy Markdown

@arthurschreiber good point, remotes are the correct place to add credentials.

Since this is merged, will this be added to remotes in a separate pull request? I would like to help out here, but my c skills are nonexistent.

@arthurschreiber

Copy link
Copy Markdown
Member Author

@dangerp Yes, I'll ad the missing functionality in a separate pull request. I've been sick all of last week, so I've been a bit behind on this.

@arthurschreiber
arthurschreiber deleted the arthur/credentials branch January 16, 2014 22:19
@arthurschreiber
arthurschreiber restored the arthur/credentials branch March 4, 2014 17:56
@arthurschreiber
arthurschreiber deleted the arthur/credentials branch March 4, 2014 17:56
@arthurschreiber
arthurschreiber restored the arthur/credentials branch March 4, 2014 17:56
@arthurschreiber
arthurschreiber deleted the arthur/credentials branch March 4, 2014 17:56
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.

3 participants