Skip to content
This repository was archived by the owner on Feb 18, 2024. It is now read-only.

Remove username/password support; always use tokens.#90

Merged
guybedford merged 3 commits intojspm:masterfrom
tamird:token-auth
Jun 27, 2016
Merged

Remove username/password support; always use tokens.#90
guybedford merged 3 commits intojspm:masterfrom
tamird:token-auth

Conversation

@tamird
Copy link
Copy Markdown
Contributor

@tamird tamird commented Jun 12, 2016

Fixes #89.

@tamird
Copy link
Copy Markdown
Contributor Author

tamird commented Jun 12, 2016

There's a lot of duplication here - I'm sure it can be cleaned up some, but I'd like to leave that to you @guybedford since I don't really know what I'm doing in JS :)

I tested this locally with a token that doesn't have public_repo and things worked.

@guybedford
Copy link
Copy Markdown
Member

Thanks, this looks good to me.

The thing is, we need to retain backwards-compatibility - password authentication can't just be removed! So the prompt could perhaps read Enter your GItHub username or token:, and then if it looks like a token was entered (checking character count / character types used), skip the password entry. Then to switch auth between credentials as we have currently and a token request style.

I know it's a bit of work with all that, but backwards-compatibility is the cost having users :)

@tamird
Copy link
Copy Markdown
Contributor Author

tamird commented Jun 12, 2016

OK I've refactored this a bit to reduce the duplication. Perhaps you could help with the backwards compatibility bit?

Comment thread github.js

function readNetrc(hostname) {
hostname = hostname || 'github.com';
var creds = netrc[hostname];
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.

All this deleted code needs to remain.

@tamird
Copy link
Copy Markdown
Contributor Author

tamird commented Jun 12, 2016

OK, seems to me we can remove all the code that permits the old configuration to go in, we just need to keep the ability to read the old config. Updated.

@guybedford
Copy link
Copy Markdown
Member

We can definitely support the better token approach here, but I can't merge this as the prompts are part of the API, we can't just remove them in this version as it will break documented workflows.

@tamird
Copy link
Copy Markdown
Contributor Author

tamird commented Jun 18, 2016

Ah I might just revert the prompts. I'll let you know when I've done that.
On Jun 18, 2016 19:49, "Guy Bedford" notifications@github.com wrote:

We can definitely support the better token approach here, but I can't
merge this as the prompts are part of the API, we can't just remove them in
this version as it will break documented workflows.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#90 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABdsPG9AQFaGzpuLh28zc1nmNI-fEHIiks5qNIQEgaJpZM4Izqxv
.

@tamird
Copy link
Copy Markdown
Contributor Author

tamird commented Jun 22, 2016

Reverted the prompts! @guybedford PTAL

@guybedford
Copy link
Copy Markdown
Member

Thanks! This looks great.

I think we should be able to alter the configuration prompts though - where it says enter your token or username, we can check if they entered a token and then skip asking for a password and set the auth object to a token mode already.

Let me know if that makes sense?

@tamird
Copy link
Copy Markdown
Contributor Author

tamird commented Jun 27, 2016

Done.

@guybedford
Copy link
Copy Markdown
Member

Excellent, thank you again for your contributions!

@guybedford guybedford merged commit a930b27 into jspm:master Jun 27, 2016
@tamird tamird deleted the token-auth branch June 27, 2016 16:45
@tamird
Copy link
Copy Markdown
Contributor Author

tamird commented Jun 27, 2016

Thanks! Could you cut a new release with this?

@guybedford
Copy link
Copy Markdown
Member

Sure, I have done a 0.13.14.

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.

2 participants