Enable GitHub connect #1318
Enable GitHub connect #1318
Conversation
app/models/user.js
Outdated
| @@ -12,6 +12,7 @@ export default Model.extend({ | |||
| email: attr(), | |||
| firstName: attr(), | |||
| lastName: attr(), | |||
| githubAuthToken: attr(), | |||
There was a problem hiding this comment.
I'm not really sure if this is what we need here, but we need a field from the API.
github_id is an alternative and something that should be safe to render for all users. However, in github integrations, we'd still need to render the auth token for current users, if the client is talking to github directly. If the client will be doing it through our API, then we don't need it client side.
@joshsmith, I think this is worth making an issue for, to discuss. What do you think?
There was a problem hiding this comment.
My gut instinct is that we do not want to expose this kind of data to the client. The API should ideally handle any sensitive data from GitHub and determine what's exposed to the client from there. We can render out a github_id to the client (and perhaps some other GitHub user information, as well, like username and photo), but the talking between GitHub and our service should happen via the API.
If you think this needs further discussion, we can do so in an issue. I am leaning heavily towards API-first here, though.
| @@ -4,6 +4,7 @@ | |||
| seem to be used as hooks for testing only }} | |||
| <li>{{#link-to 'slugged-route' user.username class="slugged-route"}}Your profile{{/link-to}}</li> | |||
| <li>{{#link-to 'settings.profile' class="profile"}}Settings{{/link-to}}</li> | |||
| <li>{{#link-to 'settings.integrations' class="integrations"}}integrations{{/link-to}}</li> | |||
There was a problem hiding this comment.
Adding it to the menu should be good enough initially, but I'm ok with a different approach.
| </div> | ||
|
|
||
| <div class="settings"> | ||
| {{#if model.githubAuthToken}} |
There was a problem hiding this comment.
Depending on previous comment, this might be {{#if model.githubId}}.
| Your account is already connected to github. | ||
| {{else}} | ||
| {{github-connect}} | ||
| {{/if}} |
There was a problem hiding this comment.
Generally, we may want to handle this via a component. Some sort of "integration card" type of thing.
begedin
force-pushed
the
1267-enable-github-connect
branch
from
ebbb470
to
adcf3c6
Compare
joshsmith
force-pushed
the
update-github-connect
branch
from
e474429
to
aaaafb1
Compare
|
My tasks here are:
Some of this may require some API work. |
joshsmith
force-pushed
the
1267-enable-github-connect
branch
from
adcf3c6
to
ab8899e
Compare
|
Still kind of in progress. Didn't get far enough. You can finish if you'd like or I can pick it up myself tomorrow. |
| @@ -28,30 +28,30 @@ export default Route.extend(AuthenticatedRouteMixin, { | |||
| let stateValidator = get(this, 'githubState'); | |||
|
|
|||
| if (stateValidator.validate(state)) { | |||
| return this._sendRequest(code); | |||
| return this._sendRequest(code, state); | |||
There was a problem hiding this comment.
The github-state service performs the validation locally, using the session data facilities provided by ember-simple-auth, so at the moment, there is no need to send the state to the API.
We discussed that as an option, but I don't think it's MVP and plenty of other libraries do such validation locally. I don't think it falls into the "should our client talk to github directly or through our API" story.
There was a problem hiding this comment.
I added this to the API and it works. GitHub does some state validation itself, apparently. At least that's how I read the docs. Can you correct me if I'm wrong on this?
There was a problem hiding this comment.
The idea is
- We hit the github authorize URL with a
redirect_toparam and astateparam - On the authorization URL, if the user accepts, we are redirected back to whichever page is specified in
redirect_to, with acodeparam and astateparam. - Our app is then obligated to check the
stateparam against the original. Since this is a cross-site navigation, we need to store the original somehow and compare against it. In our case, this is done using the session data facilities provided byember-simple-auth(which internally uses local storage), abstracted through ourgithub-stateservice. In other cases, it could be some other form of local storage, a cookie, or a server-stored value.
begedin
force-pushed
the
1267-enable-github-connect
branch
from
e8f957f
to
e317274
Compare
|
@joshsmith I've fixed and added missing tests. Only one acceptance test is failing now, which is the one related to the added |
begedin
force-pushed
the
1267-enable-github-connect
branch
from
ff046c2
to
9adb654
Compare
|
@joshsmith Looked into this further and it really does seem like we're supposed to send the |
begedin
force-pushed
the
1267-enable-github-connect
branch
2 times, most recently
from
0343f85
to
99cbd5c
Compare
- Fixed the GitHub connect authorization flow - Added settings.integrations route with github connect button - Added new components and styles - Added state - Fixed tests Fixed issue with maximum callstack size error, by switching to ember-test-selectors
joshsmith
force-pushed
the
1267-enable-github-connect
branch
from
9b61c6a
to
0b27155
Compare
Will close #1267
The base of this is currently set to be #1317, so the changes are clearer. Should be changed to develop once that is merged.
I've added a new
settings.integrationsroute. It's been added to the user menu. The layout is basic and needs work, but this gives a good base to work off of, with tests and infrastructure already written.