Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to import basic account information from GitHub. #80

Merged

Conversation

@jankeromnes
Copy link
Member

jankeromnes commented Jun 20, 2017

When enabling the GitHub integration, the following profile data is imported:

  • Username (public)
  • Full name (public)
  • SSH public keys (public)

We also plan to import verified email addresses in the future in order to
de-duplicate user accounts.

@jankeromnes jankeromnes force-pushed the jankeromnes:github-integration branch 3 times, most recently from 3ac7a03 to 6f69c74 Jun 20, 2017
@jankeromnes
Copy link
Member Author

jankeromnes commented Jun 20, 2017

@bnjbvr In this commit, I introduce:

  • A /lib/github.js module implementing GitHub's API.
  • An /admin/integrations/ panel to set up the GitHub integration for the whole website (with Client ID and Secret)
  • A "User credentials" API to manage any saved user credentials (it only supports deleting github and cloud9 credentials, but in the future we'll add/remove SSH keys with it)
  • A "Connect" button in /settings/integrations/ to import various user info from GitHub (username, public SSH keys, also real name if you didn't specify it in /settings/account/)
  • A "Disconnect" button to destroy any imported data (except real name because it's managed in /settings/account/)

A few notes:

  • Although it would take just a few more lines, we don't support "Sign in with GitHub" yet, because the Alpha version is still invite-only (no need to take users through an OAuth2 round-trip only to tell them "Sorry you need an invite").
  • The accessToken we get from GitHub will also help configure user containers for GitHub contributions in the future (we could pre-configure the hub utility, e.g. by pre-generating a GitHub Personal Access Token)

Could you please review this commit?

@jankeromnes jankeromnes requested a review from bnjbvr Jun 20, 2017
@bnjbvr
bnjbvr approved these changes Jul 7, 2017
Copy link
Contributor

bnjbvr left a comment

A few comments, looks great overall. I don't see any major flaw, so r+. Please make sure to have security people review this too. It was a fun pull request!


default:
response.statusCode = 404;
response.json({ error: 'Credentials not found' }, null, 2);

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

I think the code and error messages are misleading; if somebody writing a client made a typo / misnamed things (e.g. thinking the type for cloud9 is c9), they won't get a chance to find out. How about the 400 error code and "unknown query type"?

function load () {
const oauth2providers = db.get('oauth2providers');

// Add an non-functional, empty GitHub OAuth2 provider by default.

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

nit: a non-functional

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

Also, why loading dysfunctional dummy credentials, when you could just not load anything at all (and check presence of github property to see if the github backend has been enabled)?

This comment has been minimized.

@jankeromnes

jankeromnes Aug 30, 2017 Author Member

I prefer explicitly checking for whichever information is needed, so the check will probably always look like github && github.id && github.secret. Also, I add a stub github entry to make this feature discoverable in db.json (but you could argue that since I'm adding an admin section to enable this integration, being transparent in db.json is probably not useful).

Would you prefer I don't add an empty github credentials set?

This comment has been minimized.

@bnjbvr

bnjbvr Sep 12, 2017 Contributor

(sorry I don't recall the state of the code before this, and Github won't allow me to give a look, so I can't really answer)

const db = require('./db');
const oauth2 = require('./oauth2');

load();

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

nit: call before definition

This comment has been minimized.

@jankeromnes

jankeromnes Sep 5, 2017 Author Member

It's a common pattern in this code base to leverage hoisting in order to make it obvious when a module runs some code directly on load.

@@ -15,7 +15,7 @@
<small>Use the stunning Cloud9 IDE to hack on your projects</small>
</div>
<div class="list-group-item-controls">
<a class="btn btn-default" href="/settings/account/"><span class="hidden-xs">Go to </span>Account</a>
<a class="btn btn-default" href="/settings/account/">Manage<span class="hidden-xs"> in Account</span></a>

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

nit: manage account?

user.keys.github.accessToken = accessToken;
user.keys.github.refreshToken = refreshToken;
if (!user.profile.name && name) {
user.profile.name = name;

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

(different style but does the same: user.profile.name = user.profile.name || name;)

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

In addition to the comment in github.js about github.getProfile, you could have the name normalization done there, so all the callers don't have to repeat it.

This comment has been minimized.

@jankeromnes

jankeromnes Sep 5, 2017 Author Member

I believe that opportunistically defining the user's real name from their GitHub profile is a task for users.js, and not github.js (because it touches user internals). Also, I don't think that other callers of github.getProfile would perform the same action. So I'll leave it here.

};

// Forget a user's GitHub account details, including any access tokens.
exports.destroyGitHubInfo = function (user) {

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

Considering what it does and how it's used (viz. in the migration path), how about resetGitHubInfo?

This comment has been minimized.

@jankeromnes

jankeromnes Sep 4, 2017 Author Member

Hm, currently in most of the code base, a function called resetSomething will not simply delete the original value(s), but replace them with new value(s), e.g.

  • hosts.resetOAuth2ClientSecret
  • configurations.resetToDefault
  • resetAllCertificates
  • resetClientCertificate
  • resetServerCertificate

However, github.destroyGitHubInfo (I've renamed it locally to github.destroyGitHubAccount) simply deletes every value we hold, most notably the access token (I believe we'd expect a github.resetGitHubInfo to somehow produce a new valid accessToken to replace the old one, rather than simply delete it).

Furthermore, I like the reassuring word destroy for things like potentially sensitive access credentials (you're not simply rotating a secret, you're actually destroying it so that nothing valuable is left).

return;
}

callback(null, keyinfo);

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

Fwiw, all this code is equivalent to:

exports.request('POST', '/user/keys', data, accessToken, callback);

But it does document what's returned, so that could be useful to keep (if you decide to keep the function at all).

This comment has been minimized.

@jankeromnes

jankeromnes Sep 5, 2017 Author Member

Removed the method.

return;
}

delete oauth2States[session.id];

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

Don't you want to delete the oauth2 state in the error case too?

This comment has been minimized.

@jankeromnes

jankeromnes Sep 4, 2017 Author Member

The purpose of an OAuth2 state parameter is to prevent a repeated successful authentication from producing infinite access tokens (otherwise an attacker could simply listen for a successful OAuth2 handshake, and repeat the packets while stealing the output). Hence, it's not necessary to delete the OAuth2 state when an error happens, but only when a valid OAuth2 access token is produced from it.

exports.getAuthorizationUrl = function (request, callback) {
const { session } = request;
if (!session || !session.id) {
callback(new Error('Request has no associated session'));

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

nit: sessions (in English, everything which is not one is plural)

This comment has been minimized.

@jankeromnes

jankeromnes Aug 30, 2017 Author Member

There can be at most one session per request (but there can be several requests per session), so the singular should be correct.

This comment has been minimized.

@jankeromnes

jankeromnes Aug 30, 2017 Author Member

Ah wait, I see what you mean, but I think this is more like Request has not one session associated to it as opposed to Request has zero sessions associated to it. Hence I believe the current form is correct.

app.js Outdated
}

users.refreshGitHubInfo(user, accessToken, refreshToken);
routes.redirect(response, '/');

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

The only way to login is from the settings page, how about redirecting there?

This comment has been minimized.

@jankeromnes

jankeromnes Sep 5, 2017 Author Member

Great idea! And good catch, the flow was somewhat broken because of this. Let's redirect to the Integrations page for now. 👍

@jankeromnes
Copy link
Member Author

jankeromnes commented Jul 7, 2017

🎉🎉🎉🎆🎆🎆🎆🎆🎆🎉🎉🎉

Thanks a lot for doing this review!! 😄👍 I owe you one.

}

// Don't trust un-verified email addresses.
emails = emails.filter(email => email.verified);

This comment has been minimized.

@bnjbvr

bnjbvr Jul 7, 2017 Contributor

(an example of the reuse of a function's parameter as said on IRC, which is pretty bad for perf as far as i recall -- note it's ok as long as it's not in critical paths)

@jankeromnes jankeromnes self-assigned this Jul 13, 2017
When enabling the GitHub integration, the following profile data is imported:
- Username (public)
- Full name (public)
- SSH public keys (public)

We also plan to import verified email addresses in the future in order to
de-duplicate user accounts.

If any of this data collection seem unreasonable to you, please file an issue:
https://github.com/janitortechnology/janitor/issues/new
@jankeromnes jankeromnes force-pushed the jankeromnes:github-integration branch from 6f69c74 to 6639985 Sep 5, 2017
@jankeromnes jankeromnes merged commit 033f6e7 into JanitorTechnology:master Sep 5, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jankeromnes jankeromnes deleted the jankeromnes:github-integration branch Sep 5, 2017
@jankeromnes
Copy link
Member Author

jankeromnes commented Sep 5, 2017

Woot! 🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.