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 into from
Jun 27, 2016
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2442f15
Remove username/password support; always use tokens
tamird Jun 12, 2016
3cc3985
[TBS] revert the prompts
tamird Jun 22, 2016
5b3428d
[TBS] allow tokens to be entered
tamird Jun 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 48 additions & 43 deletions github.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,11 @@ catch(e) {}
var execGit = require('./exec-git');

function createRemoteStrings(auth, hostname) {
var authString = auth ? (encodeURIComponent(auth.username) + ':' + encodeURIComponent(auth.password) + '@') : '';
var authString = auth.username ? (encodeURIComponent(auth.username) + ':' + encodeURIComponent(auth.password) + '@') : '';
hostname = hostname || 'github.com';

this.remoteString = 'https://' + authString + hostname + '/';
this.authSuffix = auth.token ? '?access_token=' + auth.token : '';

if (hostname == 'github.com')
this.apiRemoteString = 'https://' + authString + 'api.github.com/';
Expand All @@ -46,10 +47,6 @@ function createRemoteStrings(auth, hostname) {
this.apiRemoteString = 'https://' + authString + hostname + '/api/v3/';
}

// avoid storing passwords as plain text in config
function encodeCredentials(auth) {
return new Buffer(encodeURIComponent(auth.username) + ':' + encodeURIComponent(auth.password)).toString('base64');
}
function decodeCredentials(str) {
var auth = new Buffer(str, 'base64').toString('ascii').split(':');

Expand Down Expand Up @@ -81,6 +78,10 @@ function readNetrc(hostname) {
}
}

function isGithubToken(token) {
return token.match(/[0-9a-f]{40}/);
}

var GithubLocation = function(options, ui) {

// ensure git is installed
Expand All @@ -98,19 +99,15 @@ var GithubLocation = function(options, ui) {
this.versionString = options.versionString + '.1';

// Give the environment precedence over options object
if(process.env.JSPM_GITHUB_AUTH_TOKEN) {
options.auth = process.env.JSPM_GITHUB_AUTH_TOKEN;
} else if (options.username && !options.auth) {
options.auth = encodeCredentials(options);
// NB deprecate old auth eventually
// delete options.username;
// delete options.password;
}
var auth = process.env.JSPM_GITHUB_AUTH_TOKEN || options.auth;

if (typeof options.auth == 'string') {
this.auth = decodeCredentials(options.auth);
}
else {
if (auth) {
if (isGithubToken(auth)) {
this.auth = { token: auth };
} else {
this.auth = decodeCredentials(auth);
}
} else {
this.auth = readNetrc(options.hostname);
}

Expand Down Expand Up @@ -148,7 +145,7 @@ var GithubLocation = function(options, ui) {

this.remote = options.remote;

createRemoteStrings.call(this, this.auth, options.hostname);
createRemoteStrings.call(this, this.auth || {}, options.hostname);
};

function clearDir(dir) {
Expand Down Expand Up @@ -199,20 +196,27 @@ function configureCredentials(config, ui) {

return Promise.resolve()
.then(function() {
ui.log('info', 'If using two-factor authentication or to avoid using your password you can generate an access token at %https://' + (config.hostname || 'github.com') + '/settings/tokens%. Ensure it has `public_repo` scope access.');
return ui.input('Enter your GitHub username');
ui.log('info', 'If using two-factor authentication or to avoid using your password you can generate an access token at %https://' + (config.hostname || 'github.com') + '/settings/tokens%.');
return ui.input('Enter your GitHub username or access token');
})
.then(function(username) {
auth.username = username;
if (auth.username)
return ui.input('Enter your GitHub password or access token', null, true);
.then(function(entered) {
if (!entered) {
return false;
} else if (isGithubToken(entered)) {
auth.token = entered;
} else {
auth.username = entered;
return ui.input('Enter your GitHub password', null, true);
}
})
.then(function(password) {
auth.password = password;
if (!auth.username)
return false;
if (password) {
auth.password = password;
}
Copy link
Copy Markdown
Member

@guybedford guybedford Jun 12, 2016

Choose a reason for hiding this comment

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

Again keep this code, with the prompt being Enter your GitHub username or access token as the first question, with the isGithubToken resulting in the new path (skipping password prompt), and otherwise the old path.

Copy link
Copy Markdown
Contributor Author

@tamird tamird Jun 12, 2016

Choose a reason for hiding this comment

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

Left this as-is (with the old code removed) since we don't need to allow this old stuff to go in.

Copy link
Copy Markdown
Member

@guybedford guybedford Jun 12, 2016

Choose a reason for hiding this comment

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

Backwards compat should really mean the same prompts too I think.
On Sun, 12 Jun 2016 at 02:49, Tamir Duberstein notifications@github.com
wrote:

In github.js #90 (comment)
:

})

  • .then(function(username) {
  • auth.username = username;
  • if (auth.username)
  •  return ui.input('Enter your GitHub password or access token', null, true);
  • })
  • .then(function(password) {
  • auth.password = password;
  • if (!auth.username)

- return false;

  • return ui.confirm('Would you like to test these credentials?', true);

Left this as-is (with the old code removed) since we don't need to allow
this old stuff to go in.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/jspm/github/pull/90/files/7e73f5e239dac1f8eca852495dc8029f82991f26#r66714112,
or mute the thread
https://github.com/notifications/unsubscribe/AAkiyrxzLDoQA_BvR-jvnJFmFE3VFUQcks5qK1eZgaJpZM4Izqxv
.

Copy link
Copy Markdown
Contributor Author

@tamird tamird Jun 12, 2016

Choose a reason for hiding this comment

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

Why? That means none of this can ever be deprecated or removed. We want to make sure people's existing configuration works, but why does that mean we need to support both forms forever?

That also creates a more confusing situation with the access tokens - if you use username/password you need to give public_repo, and otherwise you don't. Doesn't seem great

Copy link
Copy Markdown
Member

@guybedford guybedford Jun 12, 2016

Choose a reason for hiding this comment

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

Because the prompts form part of the public API.

If the PR was against the 0.17 branch for the jspm beta then we can
definitely consider doing that for 0.17. But on the 0.16 branch they should
remain as users expect now to be able to directly enter their github
username and password (which allows not having to go to the website to
create a token at all)
On Sun, 12 Jun 2016 at 02:54, Tamir Duberstein notifications@github.com
wrote:

In github.js #90 (comment)
:

})

  • .then(function(username) {
  • auth.username = username;
  • if (auth.username)
  •  return ui.input('Enter your GitHub password or access token', null, true);
  • })
  • .then(function(password) {
  • auth.password = password;
  • if (!auth.username)

- return false;

  • return ui.confirm('Would you like to test these credentials?', true);

Why? That means none of this can ever be deprecated or removed. We want to
make sure people's existing configuration works, but why does that mean we
need to support both forms forever?

That also creates a more confusing situation with the access tokens - if
you use username/password you need to give public_repo, and otherwise you
don't. Doesn't seem great

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/jspm/github/pull/90/files/7e73f5e239dac1f8eca852495dc8029f82991f26#r66714150,
or mute the thread
https://github.com/notifications/unsubscribe/AAkiykjKrj7oHuNSqxdXtl8EFq_VgRjXks5qK1izgaJpZM4Izqxv
.

Copy link
Copy Markdown
Contributor Author

@tamird tamird Jun 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Opened #91.


return ui.confirm('Would you like to test these credentials?', true);
if (auth.username || auth.token) {
return ui.confirm('Would you like to test these credentials?', true);
}
})
.then(function(test) {
if (!test)
Expand All @@ -224,7 +228,7 @@ function configureCredentials(config, ui) {
createRemoteStrings.call(remotes, auth, config.hostname);

return asp(request)({
uri: remotes.apiRemoteString + 'user',
uri: remotes.apiRemoteString + 'user' + remotes.authSuffix,
headers: {
'User-Agent': 'jspm',
'Accept': 'application/vnd.github.v3+json'
Expand Down Expand Up @@ -254,10 +258,10 @@ function configureCredentials(config, ui) {
.then(function(redo) {
if (redo)
return configureCredentials(config, ui);
return encodeCredentials(auth);
return auth.token;
});
else if (auth.username)
return encodeCredentials(auth);
else if (auth.token)
return auth.token;
else
return null;
});
Expand Down Expand Up @@ -328,14 +332,15 @@ GithubLocation.prototype = {
locate: function(repo) {
var self = this;
var remoteString = this.remoteString;
var authSuffix = this.authSuffix;

if (repo.split('/').length !== 2)
throw "GitHub packages must be of the form `owner/repo`.";

// request the repo to check that it isn't a redirect
return new Promise(function(resolve, reject) {
request(extend({
uri: remoteString + repo,
uri: remoteString + repo + authSuffix,
headers: {
'User-Agent': 'jspm'
},
Expand Down Expand Up @@ -433,7 +438,7 @@ GithubLocation.prototype = {
version = 'v' + version;

return asp(request)(extend({
uri: this.apiRemoteString + 'repos/' + repo + '/contents/package.json',
uri: this.apiRemoteString + 'repos/' + repo + '/contents/package.json' + this.authSuffix,
headers: {
'User-Agent': 'jspm',
'Accept': 'application/vnd.github.v3.raw'
Expand Down Expand Up @@ -538,6 +543,7 @@ GithubLocation.prototype = {
var execOpt = this.execOpt;
var max_repo_size = this.max_repo_size;
var remoteString = this.remoteString;
var authSuffix = this.authSuffix;

var self = this;

Expand Down Expand Up @@ -640,16 +646,12 @@ GithubLocation.prototype = {

// now that the inPipe is ready, do the request
request(extend({
uri: release.url,
uri: release.url + authSuffix,
headers: {
'accept': 'application/octet-stream',
'user-agent': 'jspm'
},
followRedirect: false,
auth: self.auth && {
user: self.auth.username,
pass: self.auth.password
}
}, self.defaultRequestOptions
)).on('response', function(archiveRes) {
var rateLimitResponse = checkRateLimit.call(this, archiveRes.headers);
Expand All @@ -660,7 +662,8 @@ GithubLocation.prototype = {
return reject('Bad response code ' + archiveRes.statusCode + '\n' + JSON.stringify(archiveRes.headers));

request(extend({
uri: archiveRes.headers.location, headers: {
uri: archiveRes.headers.location + authSuffix,
headers: {
'accept': 'application/octet-stream',
'user-agent': 'jspm'
}
Expand Down Expand Up @@ -692,8 +695,10 @@ GithubLocation.prototype = {
// Download from the git archive
return new Promise(function(resolve, reject) {
request(extend({
uri: remoteString + repo + '/archive/' + version + '.tar.gz',
headers: { 'accept': 'application/octet-stream' }
uri: remoteString + repo + '/archive/' + version + '.tar.gz' + authSuffix,
headers: {
'accept': 'application/octet-stream'
},
}, self.defaultRequestOptions
))
.on('response', function(pkgRes) {
Expand Down Expand Up @@ -730,7 +735,7 @@ GithubLocation.prototype = {
checkReleases: function(repo, version) {
// NB cache this on disk with etags
var reqOptions = extend({
uri: this.apiRemoteString + 'repos/' + repo + '/releases',
uri: this.apiRemoteString + 'repos/' + repo + '/releases' + this.authSuffix,
headers: {
'User-Agent': 'jspm',
'Accept': 'application/vnd.github.v3+json'
Expand Down