Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Authenticate like the Heroku CLI #176

Merged
merged 1 commit into from
Oct 15, 2014
Merged

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Sep 29, 2014

The Heroku API accepts a number of different authentication schemes:

  1. Authorization: Bearer <token>
  2. Authorization: Basic <base64 encoded ":<token>">
  3. Authorization: Basic <base64 encoded "<email>:<token>">
  4. Authorization: Basic <base64 encoded "<email>:<password>">

Currently, the CLI uses the scheme 2 while hk uses scheme 3. This pull changes
hk over to the same scheme used by the CLI (that's scheme 2).

Moving over mainly carries the benefit of intermediaries being able to tell
that two authentication schemes are equivalent and consolidate them. Scheme 2
also has the nice property of being distiguishable from scheme 4, which helps
intermediaries differentiate token-based authentication from password-based.

I'll open an equivalent pull on the heroku.go dependency. bgentry/heroku-go#13

The Heroku API accepts a number of different authentication schemes:

1. `Authorization: Bearer <Token>`
2. `Authorization: Basic <base64 encoded ":<token>">`
3. `Authorization: Basic <base64 encoded "<email>:<token>">`
4. `Authorization: Basic <base64 encoded "<email>:<password>">`

Currently, the CLI uses the scheme 2 while hk uses scheme 3. This pull
changes hk over to the same scheme used by the CLI (that's scheme 2).

Moving over mainly carries the benefit over intermediaries being able to
tell that two authentication schemes are equivalent and consolidate
them. Scheme 2 also has the nice side property of being distiguishable
from scheme 4, which helps intermediaries differentiate token-based
authentication from password-based.

I'll open an equivalent pull on the heroku.go dependency.
brandur added a commit to brandur/heroku-go that referenced this pull request Sep 29, 2014
This moves heroku.go over to the same authentication scheme as the
Heroku CLI, which allows intermediaries to recognize when equivalent
credentials are in use between different clients (say the CLI and hk for
example).

See heroku/hk#176 for more detailed information.
@@ -152,7 +152,7 @@ func (c *Client) NewRequest(method, path string, body interface{}) (*http.Reques
if ctype != "" {
req.Header.Set("Content-Type", ctype)
}
req.SetBasicAuth(c.Username, c.Password)
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes inside Godeps/_workspace should not be made by hand. If this change happens as-is, the code here won't match the commit hash listedn in Godeps.json. Instead, you can run godep update github.com/bgentry/heroku-go to pick up your new commit. (You don't have to wait for the change to be merged upstream.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kr Ah, +1! I suspected that what I was doing was poor practice.

(You don't have to wait for the change to be merged upstream.)

But my commits won't even be in that repo yet I think? Do you mean that I should run a godep update pointing to my own fork? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. Add your fork as another remote in $GOPATH/src/github.com/bgentry/heroku-go, and make your changes there, so you keep the original import path.

This is not quite ideal, since another person wouldn't be able to run godep restore, so it's best when you know there's a good chance the change will be merged upstream. People have also suggested having godep record alternate git sources so that unmerged changes will be reachable by all users, but until then at least it's somewhat ameliorated by having the source code itself present.

(If you want to really fork the project, as in going your separate ways from the original project and never merging upstream, that would be a good time to change the import path.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kr Ah! Thanks a lot for explaining, I think I understand the general process now.

@dickeyxxx If it's all the same to you, I'll correct my little Godep faux pas on master. I tried running the update, but because it pulls in a lot of other changes that occurred in heroku-go, it makes this diff about 100x larger, with very little of it relevant to the changes proposed.

Copy link

Choose a reason for hiding this comment

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

it's all the same to me, but I'm curious about the future of heroku-go. @bgentry is that something you'd like to continue owning?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dickeyxxx I don't think my version of heroku-go is something that should be carried forward indefinitely. Its generator is far inferior to the one produced by @cyberdelia, and I don't plan on rewriting mine, so his heroku-go should ultimately be the way forward. So that's my recommendation.

Copy link

Choose a reason for hiding this comment

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

thanks for the honest answer, you're referring to this? https://github.com/cyberdelia/heroku-go

Copy link

Choose a reason for hiding this comment

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

no need to reply @bgentry. @cyberdelia and I should have this from here

@brandur
Copy link
Contributor Author

brandur commented Oct 10, 2014

@dickeyxxx What do you think about pulling this one in for the time being? Thanks!

jdx pushed a commit that referenced this pull request Oct 15, 2014
@jdx jdx merged commit f5430ff into master Oct 15, 2014
@jdx jdx deleted the brandur-authenticate-like-toolbelt branch October 15, 2014 01:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants