This repository has been archived by the owner on Dec 15, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 393
Populate co-authors from mentionable users from the GitHub API #1476
Merged
Merged
Changes from all commits
Commits
Show all changes
68 commits
Select commit
Hold shift + click to select a range
ac04a71
Return canned GraphQL query responses in spec mode
smashwilson 88c48ff
Spec for the simple case of loading mentionable users
smashwilson 92b063f
Load users from GraphQL if a GitHub remote and token are present
smashwilson 5bea3ad
Assert against mentionable users that aren't also in git history
smashwilson 5f850b4
Match expected GraphQL queries by variables
smashwilson 350ff6e
Fetch multiple pages of mentionable users
smashwilson 3e9bc31
Infer no-reply emails
smashwilson d8e512e
Call addUsers once
smashwilson 7f7d07f
Author model
smashwilson 45f24ac
Construct Authors from Present.getAuthors()
smashwilson f845cdf
Return an Author or nullAuthor from getCommitter()
smashwilson 19f9d9f
Sort and match Authors
smashwilson 00ae45c
Store Authors sorted within the UserStore
smashwilson 9576c1f
Use a ModelObserver to track the Repository
smashwilson 0a6a903
Turn UserStore into an event Emitter
smashwilson e4f92a8
PropType shapes for new models
smashwilson edf3bb1
getAuthors() stub should return []
smashwilson 7ee56cb
The UserStore observes its Repository
smashwilson c4292de
Update the UserStore in the updateSelectedCoAuthors callback
smashwilson e7772b5
Pass the UserStore through the component tree
smashwilson 65066a3
Observe the passed UserStore and update the Select component
smashwilson ea65ac0
Pass an empty UserStore in tests
smashwilson c9e0883
Import shuffle
smashwilson b9b9705
I... have no idea how this was passing before?
smashwilson 3985f55
Use the Author model in the test fixture
smashwilson 0c64139
:fire: unused import
smashwilson 1d84a61
Present talks model objects, GSOS talks raw objects
smashwilson 68ea76c
More Author model usage
smashwilson f9e7146
Another Author model
smashwilson c8c6051
Acquire the token from the GithubLoginModel
smashwilson 138dadc
Track the last source of users in a UserStore
smashwilson bb734d4
:fire: console.logs
smashwilson 6bf5413
Override results when the Repository has changed
smashwilson 92a2cfa
Pass the GithubLoginModel to the UserStore
smashwilson 79b10a1
Declare query variables
smashwilson 4f8906a
Change the loginModel
smashwilson 06c073a
Create and test for new Authors
smashwilson 423f258
Create Author instances in CoAuthorForm
smashwilson 137f61a
Use Author models in the CommitView co-author forms
smashwilson 90394fc
Update CoAuthor form test
smashwilson 25c3edd
:fire: console.log again
smashwilson db7e78f
Create a helper for stubbing paginated data
smashwilson 0d3b316
getSlug() on Remote because "slug" is more fun than "nwo"
smashwilson 25095ca
Disable stubbed GraphQL queries
smashwilson d687e7b
Cache GraphQL responses for an hour
smashwilson 91e8819
Merge branch 'master' into aw/mentionable-users
smashwilson 3d8073f
Check a token's OAuth scopes against the required ones on each getToken
smashwilson 875540b
Handle an INSUFFICIENT result in UserStore
smashwilson b5c49c6
Stub getScopes in UserStore tests
smashwilson 29e8590
Log GraphQL expected variables in spec mode
smashwilson 6faf54e
Handle errors during the getScopes() request
smashwilson 744c267
Handle the "insufficient scopes" case in RemotePrController
smashwilson 7c24b6b
Oh GithubLoginView already lets you customize a message
smashwilson dbf5ac9
Report GraphQL errors from non-200 responses
smashwilson 930e89f
Don't lose the token on every launch :eyes:
smashwilson e752302
Autocomplete on login
smashwilson cbf56ab
Render a handle if we have one
smashwilson 1948962
Our tokens have read:org
smashwilson cc96a5d
Merge branch 'master' into aw/mentionable-users
smashwilson 5210a78
Merge branch 'master' into aw/mentionable-users
smashwilson f9f6ac7
Pass UNAUTHENTICATED results through GithubLoginModel
smashwilson 240c754
Respect an excludedUsers config setting
smashwilson f2610c4
Include schema for excludedUsers
smashwilson 086ee0a
Pass config to the UserStore initializer
smashwilson 1ec5ed9
Gracefully handle GraphQL errors in the UserStore
smashwilson a82ce61
Exclude co-authors on shift-delete
smashwilson 3c9b2ca
Merge branch 'master' into aw/mentionable-users
smashwilson dd96476
Initialize UserStore correctly in tests
smashwilson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
const NEW = Symbol('new'); | ||
|
||
export const NO_REPLY_GITHUB_EMAIL = 'noreply@github.com'; | ||
|
||
export default class Author { | ||
constructor(email, fullName, login = null, isNew = null) { | ||
this.email = email; | ||
this.fullName = fullName; | ||
this.login = login; | ||
this.new = isNew === NEW; | ||
} | ||
|
||
static createNew(email, fullName) { | ||
return new this(email, fullName, null, NEW); | ||
} | ||
|
||
getEmail() { | ||
return this.email; | ||
} | ||
|
||
getFullName() { | ||
return this.fullName; | ||
} | ||
|
||
getLogin() { | ||
return this.login; | ||
} | ||
|
||
isNoReply() { | ||
return this.email === NO_REPLY_GITHUB_EMAIL; | ||
} | ||
|
||
hasLogin() { | ||
return this.login !== null; | ||
} | ||
|
||
isNew() { | ||
return this.new; | ||
} | ||
|
||
isPresent() { | ||
return true; | ||
} | ||
|
||
matches(other) { | ||
return this.getEmail() === other.getEmail(); | ||
} | ||
|
||
toString() { | ||
let s = `${this.fullName} <${this.email}>`; | ||
if (this.hasLogin()) { | ||
s += ` @${this.login}`; | ||
} | ||
return s; | ||
} | ||
|
||
static compare(a, b) { | ||
if (a.getFullName() < b.getFullName()) { return -1; } | ||
if (a.getFullName() > b.getFullName()) { return 1; } | ||
return 0; | ||
} | ||
} | ||
|
||
export const nullAuthor = { | ||
getEmail() { | ||
return ''; | ||
}, | ||
|
||
getFullName() { | ||
return ''; | ||
}, | ||
|
||
getLogin() { | ||
return null; | ||
}, | ||
|
||
isNoReply() { | ||
return false; | ||
}, | ||
|
||
hasLogin() { | ||
return false; | ||
}, | ||
|
||
isNew() { | ||
return false; | ||
}, | ||
|
||
isPresent() { | ||
return false; | ||
}, | ||
|
||
matches(other) { | ||
return other === this; | ||
}, | ||
|
||
toString() { | ||
return 'null author'; | ||
}, | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,15 @@ | ||
import crypto from 'crypto'; | ||
import {Emitter} from 'event-kit'; | ||
|
||
import {UNAUTHENTICATED, createStrategy} from '../shared/keytar-strategy'; | ||
import {UNAUTHENTICATED, INSUFFICIENT, createStrategy} from '../shared/keytar-strategy'; | ||
|
||
let instance = null; | ||
|
||
export default class GithubLoginModel { | ||
// Be sure that we're requesting at least this many scopes on the token we grant through github.atom.io or we'll | ||
// give everyone a really frustrating experience ;-) | ||
static REQUIRED_SCOPES = ['repo', 'read:org', 'user:email'] | ||
|
||
static get() { | ||
if (!instance) { | ||
instance = new GithubLoginModel(); | ||
|
@@ -16,6 +21,7 @@ export default class GithubLoginModel { | |
this._Strategy = Strategy; | ||
this._strategy = null; | ||
this.emitter = new Emitter(); | ||
this.checked = new Set(); | ||
} | ||
|
||
async getStrategy() { | ||
|
@@ -34,11 +40,41 @@ export default class GithubLoginModel { | |
|
||
async getToken(account) { | ||
const strategy = await this.getStrategy(); | ||
let password = await strategy.getPassword('atom-github', account); | ||
if (!password) { | ||
const password = await strategy.getPassword('atom-github', account); | ||
if (!password || password === UNAUTHENTICATED) { | ||
// User is not logged in | ||
password = UNAUTHENTICATED; | ||
return UNAUTHENTICATED; | ||
} | ||
|
||
if (/^https?:\/\//.test(account)) { | ||
// Avoid storing tokens in memory longer than necessary. Let's cache token scope checks by storing a set of | ||
// checksums instead. | ||
const hash = crypto.createHash('md5'); | ||
hash.update(password); | ||
const fingerprint = hash.digest('base64'); | ||
|
||
if (!this.checked.has(fingerprint)) { | ||
try { | ||
const scopes = new Set(await this.getScopes(account, password)); | ||
|
||
for (const scope of this.constructor.REQUIRED_SCOPES) { | ||
if (!scopes.has(scope)) { | ||
// Token doesn't have enough OAuth scopes, need to reauthenticate | ||
return INSUFFICIENT; | ||
} | ||
} | ||
|
||
// We're good | ||
this.checked.add(fingerprint); | ||
} catch (e) { | ||
// Bad credential most likely | ||
// eslint-disable-next-line no-console | ||
console.error(`Unable to validate token scopes against ${account}`, e); | ||
return UNAUTHENTICATED; | ||
} | ||
} | ||
} | ||
|
||
return password; | ||
} | ||
|
||
|
@@ -54,6 +90,23 @@ export default class GithubLoginModel { | |
this.didUpdate(); | ||
} | ||
|
||
async getScopes(host, token) { | ||
if (atom.inSpecMode()) { | ||
throw new Error('Attempt to check token scopes in specs'); | ||
} | ||
|
||
const response = await fetch(host, { | ||
method: 'HEAD', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL -- I didn't realize |
||
headers: {Authorization: `bearer ${token}`}, | ||
}); | ||
|
||
if (response.status !== 200) { | ||
throw new Error(`Unable to check token for OAuth scopes against ${host}: ${await response.text()}`); | ||
} | ||
|
||
return response.headers.get('X-OAuth-Scopes').split(/\s*,\s*/); | ||
} | ||
|
||
didUpdate() { | ||
this.emitter.emit('did-update'); | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how hard would it be to leverage what we've built here into eventually having a unified Atom login experience? For Teletype, the GitHub package, and any other future packages that might want it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to have this so much. We'd need to find a way to make "GitHub identity" a part of the core API somehow, so that packages could just consume it. Maybe have a separate package that exists purely to handle the UI for authentication...
I'm not sure how much effort it would be. We'd need to scope it out first in an Atom RFC and get buy-in from the core team. Maybe we could wait to see how @shana can improve our authentication story first, and then generalize from there.