Use the GitHub API for all operations

[tamird]

As discussed in #99.

If this LGTY I'll open the same PR against 0.17.

@guybedford

[tamird]

cc @adamburgess

[guybedford]

Can you justify deciding to remove support for this?

[tamird]

We're no longer execing, so this make no sense now.

On Sep 6, 2016 12:26, "Guy Bedford" notifications@github.com wrote:

In github.js
#107 (comment):

this.defaultRequestOptions = {
strictSSL: 'strictSSL' in options ? options.strictSSL : true
};

• if (!this.defaultRequestOptions.strictSSL) {
• this.execOpt.env.GIT_SSL_NO_VERIFY = '1'

- }

• var self = this, envMap = {
• ca: 'GIT_SSL_CAINFO',
• cert: 'GIT_SSL_CERT',
• key: 'GIT_SSL_KEY'

Can you justify deciding to remove support for this?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/jspm/github/pull/107/files/35ce9c9ae8aeafb727b9467ef19c09f398323dbc#r77669492,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPLU1rEx9iaRUFTNoaXjwLsMBCTaoks5qnZQlgaJpZM4J2AfN
.

[guybedford]

Thanks @tamird. Unfortunately my domain knowledge here isn't up to scratch, and I can't merge this without a comprehensive review, especially after all the issues we had with the last PR.

If anyone is able to review this that would be a great help.

[maxlang]

I looked through this code and I've run test.js with both the current repo and a private repo and lookup works fine.

LGTM if this works with Github Enterprise

[adamburgess]

using only the api means that without a token, after 30 installs of a github repo it'll just die, no fallback or anything
no error messages either in this, if github errors out you tell jspm the repo wasn't found
yes, I didn't continue on my PR, but at the very least it had the fallback to git+https

[tamird]

I've added the standard api limit error messages.

In my experience, all non-trivial jspm installs without authentication fail - even after this change, trivial projects will continue to succeed and non-trivial ones will continue to fail.

The complication of the fallback doesn't, in my opinion, justify the complexity, but I'm willing to be convinced.

Reverting the original changes should be a non-starter. Requiring users to provide privileged tokens is a serious security concern.

[guybedford]

Thanks @tamird. Authentication tokens in GitHub can be made to only be read only. The only difference seems to me the inclusion of the user name, but at the cost of hurting unauthenticated workflows.

Without an alternative the best route forward is still looking like reverting that work.

[adamburgess]

@guybedford@tamird just to clarify, what work are you talking about reverting?

[tamird]

@guybedford even "Read only" tokens are overly privileged - they allow reading private repos, which should not be required for public-only jspm invocations.

[adamburgess]

@tamird so are you saying that tokens shouldn't be used and/or opt-in? because just above you said it wasn't worth it?

also from my PR, using git+https is as simple as require('./ls-remote.js') and calling it with the repo to get a promise. (though errors are not handled as best they could be, I seem to remember. Error handling is hell.)

[tamird]

@adamburgess here's the context: #90

before that PR, tokens always had to be privileged.

[adamburgess]

@tamird when creating a github token, you can opt to select no scopes at all: https://developer.github.com/v3/oauth/#scopes
-> Grants read-only access to public information (includes public user profile info, public repository info, and gists)

also, to prevent errors like #109, on setup one could check the response X-OAuth-Scopes header for repo and show a notice, e.g.:

This token does not have the repo privilege. If you wanted to use private repositories, please create a new token with that scope. Otherwise, you can safely ignore this warning.

(you could even go farther as to warn if the token given is way too privileged and has too many scopes that jspm will never use)

[tamird]

Yes, that's true, and yet it didn't work. See #89.

[adamburgess]

I don't see anything stating that it doesn't work, there

[tamird]

That issue is about unprivileged tokens not working with JSPM (they had to have public_repo, at a minimum). So your suggestion about not selecting any scopes is correct, but the old implementation (pre #90) did not work with such tokens.

[adamburgess]

but that's what this PR is for: a new implementation ;)

[tamird]

Yes, that's true. Perhaps you could open an alternative PR to flesh out
your suggestion.

On Sep 22, 2016 08:46, "Adam Burgess" notifications@github.com wrote:

but that's what this PR is for: a new implementation ;)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#107 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdsPCRX6ZwoGBn7sspHiBl5ZqNfWO_tks5qsniVgaJpZM4J2AfN
.

[tamird]

As discussed in #99.

If this LGTY I'll open the same PR against 0.17.

@guybedford

[tamird]

cc @adamburgess

[guybedford]

Can you justify deciding to remove support for this?

[tamird]

We're no longer execing, so this make no sense now.

On Sep 6, 2016 12:26, "Guy Bedford" notifications@github.com wrote:

In github.js
#107 (comment):

this.defaultRequestOptions = {
strictSSL: 'strictSSL' in options ? options.strictSSL : true
};

• if (!this.defaultRequestOptions.strictSSL) {
• this.execOpt.env.GIT_SSL_NO_VERIFY = '1'

- }

• var self = this, envMap = {
• ca: 'GIT_SSL_CAINFO',
• cert: 'GIT_SSL_CERT',
• key: 'GIT_SSL_KEY'

Can you justify deciding to remove support for this?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/jspm/github/pull/107/files/35ce9c9ae8aeafb727b9467ef19c09f398323dbc#r77669492,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPLU1rEx9iaRUFTNoaXjwLsMBCTaoks5qnZQlgaJpZM4J2AfN
.

[guybedford]

Thanks @tamird. Unfortunately my domain knowledge here isn't up to scratch, and I can't merge this without a comprehensive review, especially after all the issues we had with the last PR.

If anyone is able to review this that would be a great help.

[maxlang]

I looked through this code and I've run test.js with both the current repo and a private repo and lookup works fine.

LGTM if this works with Github Enterprise

[adamburgess]

using only the api means that without a token, after 30 installs of a github repo it'll just die, no fallback or anything
no error messages either in this, if github errors out you tell jspm the repo wasn't found
yes, I didn't continue on my PR, but at the very least it had the fallback to git+https

[tamird]

I've added the standard api limit error messages.

In my experience, all non-trivial jspm installs without authentication fail - even after this change, trivial projects will continue to succeed and non-trivial ones will continue to fail.

The complication of the fallback doesn't, in my opinion, justify the complexity, but I'm willing to be convinced.

Reverting the original changes should be a non-starter. Requiring users to provide privileged tokens is a serious security concern.

[guybedford]

Thanks @tamird. Authentication tokens in GitHub can be made to only be read only. The only difference seems to me the inclusion of the user name, but at the cost of hurting unauthenticated workflows.

Without an alternative the best route forward is still looking like reverting that work.

[adamburgess]

@guybedford@tamird just to clarify, what work are you talking about reverting?

[tamird]

@guybedford even "Read only" tokens are overly privileged - they allow reading private repos, which should not be required for public-only jspm invocations.

[adamburgess]

@tamird so are you saying that tokens shouldn't be used and/or opt-in? because just above you said it wasn't worth it?

also from my PR, using git+https is as simple as require('./ls-remote.js') and calling it with the repo to get a promise. (though errors are not handled as best they could be, I seem to remember. Error handling is hell.)

[tamird]

@adamburgess here's the context: #90

before that PR, tokens always had to be privileged.

[adamburgess]

@tamird when creating a github token, you can opt to select no scopes at all: https://developer.github.com/v3/oauth/#scopes
-> Grants read-only access to public information (includes public user profile info, public repository info, and gists)

also, to prevent errors like #109, on setup one could check the response X-OAuth-Scopes header for repo and show a notice, e.g.:

This token does not have the repo privilege. If you wanted to use private repositories, please create a new token with that scope. Otherwise, you can safely ignore this warning.

(you could even go farther as to warn if the token given is way too privileged and has too many scopes that jspm will never use)

[tamird]

Yes, that's true, and yet it didn't work. See #89.

[adamburgess]

I don't see anything stating that it doesn't work, there

[tamird]

That issue is about unprivileged tokens not working with JSPM (they had to have public_repo, at a minimum). So your suggestion about not selecting any scopes is correct, but the old implementation (pre #90) did not work with such tokens.

[adamburgess]

but that's what this PR is for: a new implementation ;)

[tamird]

Yes, that's true. Perhaps you could open an alternative PR to flesh out
your suggestion.

On Sep 22, 2016 08:46, "Adam Burgess" notifications@github.com wrote:

but that's what this PR is for: a new implementation ;)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#107 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdsPCRX6ZwoGBn7sspHiBl5ZqNfWO_tks5qsniVgaJpZM4J2AfN
.

[tamird]

As discussed in #99.

If this LGTY I'll open the same PR against 0.17.

@guybedford

[tamird]

cc @adamburgess

[guybedford]

Can you justify deciding to remove support for this?

[tamird]

We're no longer execing, so this make no sense now.

On Sep 6, 2016 12:26, "Guy Bedford" notifications@github.com wrote:

In github.js
#107 (comment):

this.defaultRequestOptions = {
strictSSL: 'strictSSL' in options ? options.strictSSL : true
};

• if (!this.defaultRequestOptions.strictSSL) {
• this.execOpt.env.GIT_SSL_NO_VERIFY = '1'

- }

• var self = this, envMap = {
• ca: 'GIT_SSL_CAINFO',
• cert: 'GIT_SSL_CERT',
• key: 'GIT_SSL_KEY'

Can you justify deciding to remove support for this?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/jspm/github/pull/107/files/35ce9c9ae8aeafb727b9467ef19c09f398323dbc#r77669492,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPLU1rEx9iaRUFTNoaXjwLsMBCTaoks5qnZQlgaJpZM4J2AfN
.

[guybedford]

Thanks @tamird. Unfortunately my domain knowledge here isn't up to scratch, and I can't merge this without a comprehensive review, especially after all the issues we had with the last PR.

If anyone is able to review this that would be a great help.

[maxlang]

I looked through this code and I've run test.js with both the current repo and a private repo and lookup works fine.

LGTM if this works with Github Enterprise

[adamburgess]

using only the api means that without a token, after 30 installs of a github repo it'll just die, no fallback or anything
no error messages either in this, if github errors out you tell jspm the repo wasn't found
yes, I didn't continue on my PR, but at the very least it had the fallback to git+https

[tamird]

I've added the standard api limit error messages.

In my experience, all non-trivial jspm installs without authentication fail - even after this change, trivial projects will continue to succeed and non-trivial ones will continue to fail.

The complication of the fallback doesn't, in my opinion, justify the complexity, but I'm willing to be convinced.

Reverting the original changes should be a non-starter. Requiring users to provide privileged tokens is a serious security concern.

[guybedford]

Thanks @tamird. Authentication tokens in GitHub can be made to only be read only. The only difference seems to me the inclusion of the user name, but at the cost of hurting unauthenticated workflows.

Without an alternative the best route forward is still looking like reverting that work.

[adamburgess]

@guybedford@tamird just to clarify, what work are you talking about reverting?

[tamird]

@guybedford even "Read only" tokens are overly privileged - they allow reading private repos, which should not be required for public-only jspm invocations.

[adamburgess]

@tamird so are you saying that tokens shouldn't be used and/or opt-in? because just above you said it wasn't worth it?

also from my PR, using git+https is as simple as require('./ls-remote.js') and calling it with the repo to get a promise. (though errors are not handled as best they could be, I seem to remember. Error handling is hell.)

[tamird]

@adamburgess here's the context: #90

before that PR, tokens always had to be privileged.

[adamburgess]

@tamird when creating a github token, you can opt to select no scopes at all: https://developer.github.com/v3/oauth/#scopes
-> Grants read-only access to public information (includes public user profile info, public repository info, and gists)

also, to prevent errors like #109, on setup one could check the response X-OAuth-Scopes header for repo and show a notice, e.g.:

This token does not have the repo privilege. If you wanted to use private repositories, please create a new token with that scope. Otherwise, you can safely ignore this warning.

(you could even go farther as to warn if the token given is way too privileged and has too many scopes that jspm will never use)

[tamird]

Yes, that's true, and yet it didn't work. See #89.

[adamburgess]

I don't see anything stating that it doesn't work, there

[tamird]

That issue is about unprivileged tokens not working with JSPM (they had to have public_repo, at a minimum). So your suggestion about not selecting any scopes is correct, but the old implementation (pre #90) did not work with such tokens.

[adamburgess]

but that's what this PR is for: a new implementation ;)

[tamird]

Yes, that's true. Perhaps you could open an alternative PR to flesh out
your suggestion.

On Sep 22, 2016 08:46, "Adam Burgess" notifications@github.com wrote:

but that's what this PR is for: a new implementation ;)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#107 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdsPCRX6ZwoGBn7sspHiBl5ZqNfWO_tks5qsniVgaJpZM4J2AfN
.

[tamird]

As discussed in #99.

If this LGTY I'll open the same PR against 0.17.

@guybedford

[tamird]

cc @adamburgess

[guybedford]

Can you justify deciding to remove support for this?

[tamird]

We're no longer execing, so this make no sense now.

On Sep 6, 2016 12:26, "Guy Bedford" notifications@github.com wrote:

In github.js
#107 (comment):

this.defaultRequestOptions = {
strictSSL: 'strictSSL' in options ? options.strictSSL : true
};

• if (!this.defaultRequestOptions.strictSSL) {
• this.execOpt.env.GIT_SSL_NO_VERIFY = '1'

- }

• var self = this, envMap = {
• ca: 'GIT_SSL_CAINFO',
• cert: 'GIT_SSL_CERT',
• key: 'GIT_SSL_KEY'

Can you justify deciding to remove support for this?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/jspm/github/pull/107/files/35ce9c9ae8aeafb727b9467ef19c09f398323dbc#r77669492,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPLU1rEx9iaRUFTNoaXjwLsMBCTaoks5qnZQlgaJpZM4J2AfN
.

[guybedford]

Thanks @tamird. Unfortunately my domain knowledge here isn't up to scratch, and I can't merge this without a comprehensive review, especially after all the issues we had with the last PR.

If anyone is able to review this that would be a great help.

[maxlang]

I looked through this code and I've run test.js with both the current repo and a private repo and lookup works fine.

LGTM if this works with Github Enterprise

[adamburgess]

using only the api means that without a token, after 30 installs of a github repo it'll just die, no fallback or anything
no error messages either in this, if github errors out you tell jspm the repo wasn't found
yes, I didn't continue on my PR, but at the very least it had the fallback to git+https

[tamird]

I've added the standard api limit error messages.

In my experience, all non-trivial jspm installs without authentication fail - even after this change, trivial projects will continue to succeed and non-trivial ones will continue to fail.

The complication of the fallback doesn't, in my opinion, justify the complexity, but I'm willing to be convinced.

Reverting the original changes should be a non-starter. Requiring users to provide privileged tokens is a serious security concern.

[guybedford]

Thanks @tamird. Authentication tokens in GitHub can be made to only be read only. The only difference seems to me the inclusion of the user name, but at the cost of hurting unauthenticated workflows.

Without an alternative the best route forward is still looking like reverting that work.

[adamburgess]

@guybedford@tamird just to clarify, what work are you talking about reverting?

[tamird]

@guybedford even "Read only" tokens are overly privileged - they allow reading private repos, which should not be required for public-only jspm invocations.

[adamburgess]

@tamird so are you saying that tokens shouldn't be used and/or opt-in? because just above you said it wasn't worth it?

also from my PR, using git+https is as simple as require('./ls-remote.js') and calling it with the repo to get a promise. (though errors are not handled as best they could be, I seem to remember. Error handling is hell.)

[tamird]

@adamburgess here's the context: #90

before that PR, tokens always had to be privileged.

[adamburgess]

@tamird when creating a github token, you can opt to select no scopes at all: https://developer.github.com/v3/oauth/#scopes
-> Grants read-only access to public information (includes public user profile info, public repository info, and gists)

also, to prevent errors like #109, on setup one could check the response X-OAuth-Scopes header for repo and show a notice, e.g.:

This token does not have the repo privilege. If you wanted to use private repositories, please create a new token with that scope. Otherwise, you can safely ignore this warning.

(you could even go farther as to warn if the token given is way too privileged and has too many scopes that jspm will never use)

[tamird]

Yes, that's true, and yet it didn't work. See #89.

[adamburgess]

I don't see anything stating that it doesn't work, there

[tamird]

That issue is about unprivileged tokens not working with JSPM (they had to have public_repo, at a minimum). So your suggestion about not selecting any scopes is correct, but the old implementation (pre #90) did not work with such tokens.

[adamburgess]

but that's what this PR is for: a new implementation ;)

[tamird]

Yes, that's true. Perhaps you could open an alternative PR to flesh out
your suggestion.

On Sep 22, 2016 08:46, "Adam Burgess" notifications@github.com wrote:

but that's what this PR is for: a new implementation ;)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#107 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdsPCRX6ZwoGBn7sspHiBl5ZqNfWO_tks5qsniVgaJpZM4J2AfN
.

[tamird]

As discussed in #99.

If this LGTY I'll open the same PR against 0.17.

@guybedford

[tamird]

cc @adamburgess

[guybedford]

Can you justify deciding to remove support for this?

[tamird]

We're no longer execing, so this make no sense now.

On Sep 6, 2016 12:26, "Guy Bedford" notifications@github.com wrote:

In github.js
#107 (comment):

this.defaultRequestOptions = {
strictSSL: 'strictSSL' in options ? options.strictSSL : true
};

• if (!this.defaultRequestOptions.strictSSL) {
• this.execOpt.env.GIT_SSL_NO_VERIFY = '1'

- }

• var self = this, envMap = {
• ca: 'GIT_SSL_CAINFO',
• cert: 'GIT_SSL_CERT',
• key: 'GIT_SSL_KEY'

Can you justify deciding to remove support for this?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/jspm/github/pull/107/files/35ce9c9ae8aeafb727b9467ef19c09f398323dbc#r77669492,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPLU1rEx9iaRUFTNoaXjwLsMBCTaoks5qnZQlgaJpZM4J2AfN
.

[guybedford]

Thanks @tamird. Unfortunately my domain knowledge here isn't up to scratch, and I can't merge this without a comprehensive review, especially after all the issues we had with the last PR.

If anyone is able to review this that would be a great help.

[maxlang]

I looked through this code and I've run test.js with both the current repo and a private repo and lookup works fine.

LGTM if this works with Github Enterprise

[adamburgess]

using only the api means that without a token, after 30 installs of a github repo it'll just die, no fallback or anything
no error messages either in this, if github errors out you tell jspm the repo wasn't found
yes, I didn't continue on my PR, but at the very least it had the fallback to git+https

[tamird]

I've added the standard api limit error messages.

In my experience, all non-trivial jspm installs without authentication fail - even after this change, trivial projects will continue to succeed and non-trivial ones will continue to fail.

The complication of the fallback doesn't, in my opinion, justify the complexity, but I'm willing to be convinced.

Reverting the original changes should be a non-starter. Requiring users to provide privileged tokens is a serious security concern.

[guybedford]

Thanks @tamird. Authentication tokens in GitHub can be made to only be read only. The only difference seems to me the inclusion of the user name, but at the cost of hurting unauthenticated workflows.

Without an alternative the best route forward is still looking like reverting that work.

[adamburgess]

@guybedford@tamird just to clarify, what work are you talking about reverting?

[tamird]

@guybedford even "Read only" tokens are overly privileged - they allow reading private repos, which should not be required for public-only jspm invocations.

[adamburgess]

@tamird so are you saying that tokens shouldn't be used and/or opt-in? because just above you said it wasn't worth it?

also from my PR, using git+https is as simple as require('./ls-remote.js') and calling it with the repo to get a promise. (though errors are not handled as best they could be, I seem to remember. Error handling is hell.)

[tamird]

@adamburgess here's the context: #90

before that PR, tokens always had to be privileged.

[adamburgess]

@tamird when creating a github token, you can opt to select no scopes at all: https://developer.github.com/v3/oauth/#scopes
-> Grants read-only access to public information (includes public user profile info, public repository info, and gists)

also, to prevent errors like #109, on setup one could check the response X-OAuth-Scopes header for repo and show a notice, e.g.:

This token does not have the repo privilege. If you wanted to use private repositories, please create a new token with that scope. Otherwise, you can safely ignore this warning.

(you could even go farther as to warn if the token given is way too privileged and has too many scopes that jspm will never use)

[tamird]

Yes, that's true, and yet it didn't work. See #89.

[adamburgess]

I don't see anything stating that it doesn't work, there

[tamird]

That issue is about unprivileged tokens not working with JSPM (they had to have public_repo, at a minimum). So your suggestion about not selecting any scopes is correct, but the old implementation (pre #90) did not work with such tokens.

[adamburgess]

but that's what this PR is for: a new implementation ;)

[tamird]

Yes, that's true. Perhaps you could open an alternative PR to flesh out
your suggestion.

On Sep 22, 2016 08:46, "Adam Burgess" notifications@github.com wrote:

but that's what this PR is for: a new implementation ;)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#107 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdsPCRX6ZwoGBn7sspHiBl5ZqNfWO_tks5qsniVgaJpZM4J2AfN
.

[tamird]

As discussed in #99.

If this LGTY I'll open the same PR against 0.17.

@guybedford

[tamird]

cc @adamburgess

[guybedford]

Can you justify deciding to remove support for this?

[tamird]

We're no longer execing, so this make no sense now.

On Sep 6, 2016 12:26, "Guy Bedford" notifications@github.com wrote:

In github.js
#107 (comment):

this.defaultRequestOptions = {
strictSSL: 'strictSSL' in options ? options.strictSSL : true
};

• if (!this.defaultRequestOptions.strictSSL) {
• this.execOpt.env.GIT_SSL_NO_VERIFY = '1'

- }

• var self = this, envMap = {
• ca: 'GIT_SSL_CAINFO',
• cert: 'GIT_SSL_CERT',
• key: 'GIT_SSL_KEY'

Can you justify deciding to remove support for this?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/jspm/github/pull/107/files/35ce9c9ae8aeafb727b9467ef19c09f398323dbc#r77669492,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPLU1rEx9iaRUFTNoaXjwLsMBCTaoks5qnZQlgaJpZM4J2AfN
.

[guybedford]

Thanks @tamird. Unfortunately my domain knowledge here isn't up to scratch, and I can't merge this without a comprehensive review, especially after all the issues we had with the last PR.

If anyone is able to review this that would be a great help.

[maxlang]

I looked through this code and I've run test.js with both the current repo and a private repo and lookup works fine.

LGTM if this works with Github Enterprise

[adamburgess]

using only the api means that without a token, after 30 installs of a github repo it'll just die, no fallback or anything
no error messages either in this, if github errors out you tell jspm the repo wasn't found
yes, I didn't continue on my PR, but at the very least it had the fallback to git+https

[tamird]

I've added the standard api limit error messages.

In my experience, all non-trivial jspm installs without authentication fail - even after this change, trivial projects will continue to succeed and non-trivial ones will continue to fail.

The complication of the fallback doesn't, in my opinion, justify the complexity, but I'm willing to be convinced.

Reverting the original changes should be a non-starter. Requiring users to provide privileged tokens is a serious security concern.

[guybedford]

Thanks @tamird. Authentication tokens in GitHub can be made to only be read only. The only difference seems to me the inclusion of the user name, but at the cost of hurting unauthenticated workflows.

Without an alternative the best route forward is still looking like reverting that work.

[adamburgess]

@guybedford@tamird just to clarify, what work are you talking about reverting?

[tamird]

@guybedford even "Read only" tokens are overly privileged - they allow reading private repos, which should not be required for public-only jspm invocations.

[adamburgess]

@tamird so are you saying that tokens shouldn't be used and/or opt-in? because just above you said it wasn't worth it?

also from my PR, using git+https is as simple as require('./ls-remote.js') and calling it with the repo to get a promise. (though errors are not handled as best they could be, I seem to remember. Error handling is hell.)

[tamird]

@adamburgess here's the context: #90

before that PR, tokens always had to be privileged.

[adamburgess]

@tamird when creating a github token, you can opt to select no scopes at all: https://developer.github.com/v3/oauth/#scopes
-> Grants read-only access to public information (includes public user profile info, public repository info, and gists)

also, to prevent errors like #109, on setup one could check the response X-OAuth-Scopes header for repo and show a notice, e.g.:

This token does not have the repo privilege. If you wanted to use private repositories, please create a new token with that scope. Otherwise, you can safely ignore this warning.

(you could even go farther as to warn if the token given is way too privileged and has too many scopes that jspm will never use)

[tamird]

Yes, that's true, and yet it didn't work. See #89.

[adamburgess]

I don't see anything stating that it doesn't work, there

[tamird]

That issue is about unprivileged tokens not working with JSPM (they had to have public_repo, at a minimum). So your suggestion about not selecting any scopes is correct, but the old implementation (pre #90) did not work with such tokens.

[adamburgess]

but that's what this PR is for: a new implementation ;)

[tamird]

Yes, that's true. Perhaps you could open an alternative PR to flesh out
your suggestion.

On Sep 22, 2016 08:46, "Adam Burgess" notifications@github.com wrote:

but that's what this PR is for: a new implementation ;)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#107 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdsPCRX6ZwoGBn7sspHiBl5ZqNfWO_tks5qsniVgaJpZM4J2AfN
.

[tamird]

As discussed in #99.

If this LGTY I'll open the same PR against 0.17.

@guybedford

[tamird]

cc @adamburgess

[guybedford]

Can you justify deciding to remove support for this?

[tamird]

We're no longer execing, so this make no sense now.

On Sep 6, 2016 12:26, "Guy Bedford" notifications@github.com wrote:

In github.js
#107 (comment):

this.defaultRequestOptions = {
strictSSL: 'strictSSL' in options ? options.strictSSL : true
};

• if (!this.defaultRequestOptions.strictSSL) {
• this.execOpt.env.GIT_SSL_NO_VERIFY = '1'

- }

• var self = this, envMap = {
• ca: 'GIT_SSL_CAINFO',
• cert: 'GIT_SSL_CERT',
• key: 'GIT_SSL_KEY'

Can you justify deciding to remove support for this?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/jspm/github/pull/107/files/35ce9c9ae8aeafb727b9467ef19c09f398323dbc#r77669492,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPLU1rEx9iaRUFTNoaXjwLsMBCTaoks5qnZQlgaJpZM4J2AfN
.

[guybedford]

Thanks @tamird. Unfortunately my domain knowledge here isn't up to scratch, and I can't merge this without a comprehensive review, especially after all the issues we had with the last PR.

If anyone is able to review this that would be a great help.

[maxlang]

I looked through this code and I've run test.js with both the current repo and a private repo and lookup works fine.

LGTM if this works with Github Enterprise

[adamburgess]

using only the api means that without a token, after 30 installs of a github repo it'll just die, no fallback or anything
no error messages either in this, if github errors out you tell jspm the repo wasn't found
yes, I didn't continue on my PR, but at the very least it had the fallback to git+https

[tamird]

I've added the standard api limit error messages.

In my experience, all non-trivial jspm installs without authentication fail - even after this change, trivial projects will continue to succeed and non-trivial ones will continue to fail.

The complication of the fallback doesn't, in my opinion, justify the complexity, but I'm willing to be convinced.

Reverting the original changes should be a non-starter. Requiring users to provide privileged tokens is a serious security concern.

[guybedford]

Thanks @tamird. Authentication tokens in GitHub can be made to only be read only. The only difference seems to me the inclusion of the user name, but at the cost of hurting unauthenticated workflows.

Without an alternative the best route forward is still looking like reverting that work.

[adamburgess]

@guybedford@tamird just to clarify, what work are you talking about reverting?

[tamird]

@guybedford even "Read only" tokens are overly privileged - they allow reading private repos, which should not be required for public-only jspm invocations.

[adamburgess]

@tamird so are you saying that tokens shouldn't be used and/or opt-in? because just above you said it wasn't worth it?

also from my PR, using git+https is as simple as require('./ls-remote.js') and calling it with the repo to get a promise. (though errors are not handled as best they could be, I seem to remember. Error handling is hell.)

[tamird]

@adamburgess here's the context: #90

before that PR, tokens always had to be privileged.

[adamburgess]

@tamird when creating a github token, you can opt to select no scopes at all: https://developer.github.com/v3/oauth/#scopes
-> Grants read-only access to public information (includes public user profile info, public repository info, and gists)

also, to prevent errors like #109, on setup one could check the response X-OAuth-Scopes header for repo and show a notice, e.g.:

This token does not have the repo privilege. If you wanted to use private repositories, please create a new token with that scope. Otherwise, you can safely ignore this warning.

(you could even go farther as to warn if the token given is way too privileged and has too many scopes that jspm will never use)

[tamird]

Yes, that's true, and yet it didn't work. See #89.

[adamburgess]

I don't see anything stating that it doesn't work, there

[tamird]

That issue is about unprivileged tokens not working with JSPM (they had to have public_repo, at a minimum). So your suggestion about not selecting any scopes is correct, but the old implementation (pre #90) did not work with such tokens.

[adamburgess]

but that's what this PR is for: a new implementation ;)

[tamird]

Yes, that's true. Perhaps you could open an alternative PR to flesh out
your suggestion.

On Sep 22, 2016 08:46, "Adam Burgess" notifications@github.com wrote:

but that's what this PR is for: a new implementation ;)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#107 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdsPCRX6ZwoGBn7sspHiBl5ZqNfWO_tks5qsniVgaJpZM4J2AfN
.

[tamird]

As discussed in #99.

If this LGTY I'll open the same PR against 0.17.

@guybedford

[tamird]

cc @adamburgess

[guybedford]

Can you justify deciding to remove support for this?

[tamird]

We're no longer execing, so this make no sense now.

On Sep 6, 2016 12:26, "Guy Bedford" notifications@github.com wrote:

In github.js
#107 (comment):

this.defaultRequestOptions = {
strictSSL: 'strictSSL' in options ? options.strictSSL : true
};

• if (!this.defaultRequestOptions.strictSSL) {
• this.execOpt.env.GIT_SSL_NO_VERIFY = '1'

- }

• var self = this, envMap = {
• ca: 'GIT_SSL_CAINFO',
• cert: 'GIT_SSL_CERT',
• key: 'GIT_SSL_KEY'

Can you justify deciding to remove support for this?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/jspm/github/pull/107/files/35ce9c9ae8aeafb727b9467ef19c09f398323dbc#r77669492,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPLU1rEx9iaRUFTNoaXjwLsMBCTaoks5qnZQlgaJpZM4J2AfN
.

[guybedford]

Thanks @tamird. Unfortunately my domain knowledge here isn't up to scratch, and I can't merge this without a comprehensive review, especially after all the issues we had with the last PR.

If anyone is able to review this that would be a great help.

[maxlang]

I looked through this code and I've run test.js with both the current repo and a private repo and lookup works fine.

LGTM if this works with Github Enterprise

[adamburgess]

using only the api means that without a token, after 30 installs of a github repo it'll just die, no fallback or anything
no error messages either in this, if github errors out you tell jspm the repo wasn't found
yes, I didn't continue on my PR, but at the very least it had the fallback to git+https

[tamird]

I've added the standard api limit error messages.

In my experience, all non-trivial jspm installs without authentication fail - even after this change, trivial projects will continue to succeed and non-trivial ones will continue to fail.

The complication of the fallback doesn't, in my opinion, justify the complexity, but I'm willing to be convinced.

Reverting the original changes should be a non-starter. Requiring users to provide privileged tokens is a serious security concern.

[guybedford]

Thanks @tamird. Authentication tokens in GitHub can be made to only be read only. The only difference seems to me the inclusion of the user name, but at the cost of hurting unauthenticated workflows.

Without an alternative the best route forward is still looking like reverting that work.

[adamburgess]

@guybedford@tamird just to clarify, what work are you talking about reverting?

[tamird]

@guybedford even "Read only" tokens are overly privileged - they allow reading private repos, which should not be required for public-only jspm invocations.

[adamburgess]

@tamird so are you saying that tokens shouldn't be used and/or opt-in? because just above you said it wasn't worth it?

also from my PR, using git+https is as simple as require('./ls-remote.js') and calling it with the repo to get a promise. (though errors are not handled as best they could be, I seem to remember. Error handling is hell.)

[tamird]

@adamburgess here's the context: #90

before that PR, tokens always had to be privileged.

[adamburgess]

@tamird when creating a github token, you can opt to select no scopes at all: https://developer.github.com/v3/oauth/#scopes
-> Grants read-only access to public information (includes public user profile info, public repository info, and gists)

also, to prevent errors like #109, on setup one could check the response X-OAuth-Scopes header for repo and show a notice, e.g.:

This token does not have the repo privilege. If you wanted to use private repositories, please create a new token with that scope. Otherwise, you can safely ignore this warning.

(you could even go farther as to warn if the token given is way too privileged and has too many scopes that jspm will never use)

[tamird]

Yes, that's true, and yet it didn't work. See #89.

[adamburgess]

I don't see anything stating that it doesn't work, there

[tamird]

That issue is about unprivileged tokens not working with JSPM (they had to have public_repo, at a minimum). So your suggestion about not selecting any scopes is correct, but the old implementation (pre #90) did not work with such tokens.

[adamburgess]

but that's what this PR is for: a new implementation ;)

[tamird]

Yes, that's true. Perhaps you could open an alternative PR to flesh out
your suggestion.

On Sep 22, 2016 08:46, "Adam Burgess" notifications@github.com wrote:

but that's what this PR is for: a new implementation ;)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#107 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdsPCRX6ZwoGBn7sspHiBl5ZqNfWO_tks5qsniVgaJpZM4J2AfN
.