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

Address issue #323 and enable GHE Auth #491

Merged
merged 10 commits into from
Aug 2, 2017
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,5 @@ npm-debug.log
.tern-project
yarn-error.log
.vscode/
manifest.yml
.imdone/
8 changes: 8 additions & 0 deletions src/actions/auth.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { currentBackend } from '../backends/backend';
import { actions as notifActions } from 'redux-notifications';

const { notifSend } = notifActions;
Copy link
Contributor

Choose a reason for hiding this comment

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

Today I learned you can't do full destructuring in an import call - I was going to suggest import { actions: { notifSend } } from 'redux-notifications';, but that syntax is only supported for assignment, not imports).


export const AUTH_REQUEST = 'AUTH_REQUEST';
export const AUTH_SUCCESS = 'AUTH_SUCCESS';
Expand Down Expand Up @@ -60,6 +63,11 @@ export function loginUser(credentials) {
dispatch(authenticate(user));
})
.catch((error) => {
dispatch(notifSend({
Copy link
Contributor

Choose a reason for hiding this comment

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

👍, this is a long-awaited change.

message: `${ error.message }`,
kind: 'warning',
dismissAfter: 8000,
}));
dispatch(authError(error));
});
};
Expand Down
17 changes: 16 additions & 1 deletion src/backends/github/API.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,19 @@ export default class API {
return this.request("/user");
}

isCollaborator(user) {
return this.request('/user/repos').then((repos) => {
let contributor = false
for (const repo of repos) {
if (repo.full_name === this.repo && repo.permissions.push) contributor = true;
}
return contributor;
}).catch((error) => {
console.error("Problem with response of /user/repos from GitHub");
throw error;
})
}

requestHeaders(headers = {}) {
const baseHeader = {
"Content-Type": "application/json",
Expand Down Expand Up @@ -67,6 +80,9 @@ export default class API {
if (contentType && contentType.match(/json/)) {
return this.parseJsonResponse(response);
}
if (responseStatus !== 200) {
Copy link
Contributor

@Benaiah Benaiah Aug 1, 2017

Choose a reason for hiding this comment

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

A generic request method like this should check the equivalent of response.ok, which is "status in the range 200-299" (response.ok on MDN, see also MDN's list of 2xx status codes). Changing it to this line would fix that:

      if ((responseStatus >= 200) && (responseStatus <= 299)) {

Copy link
Contributor Author

@tortilaman tortilaman Aug 1, 2017

Choose a reason for hiding this comment

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

@Benaiah That's leftover from the collaborator check. I switched to a write permission check, so I'll just remove that actually.

return responseStatus
}
return response.text();
})
.catch((error) => {
Expand Down Expand Up @@ -241,7 +257,6 @@ export default class API {
persistFiles(entry, mediaFiles, options) {
const uploadPromises = [];
const files = mediaFiles.concat(entry);


files.forEach((file) => {
if (file.uploaded) { return; }
Expand Down
1 change: 1 addition & 0 deletions src/backends/github/AuthenticationPage.css
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
.root {
display: flex;
flex-flow: column nowrap;
align-items: center;
justify-content: center;
height: 100vh;
Expand Down
5 changes: 4 additions & 1 deletion src/backends/github/AuthenticationPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import React from 'react';
import Button from 'react-toolbox/lib/button';
import Authenticator from '../../lib/netlify-auth';
import { Icon } from '../../components/UI';
import { Notifs } from 'redux-notifications';
import { Toast } from '../../components/UI/index';
import styles from './AuthenticationPage.css';

export default class AuthenticationPage extends React.Component {
Expand All @@ -16,7 +18,7 @@ export default class AuthenticationPage extends React.Component {
const cfg = {
base_url: this.props.base_url,
site_id: (document.location.host.split(':')[0] === 'localhost') ? 'cms.netlify.com' : this.props.siteId
}
};
const auth = new Authenticator(cfg);

auth.authenticate({ provider: 'github', scope: 'repo' }, (err, data) => {
Expand All @@ -33,6 +35,7 @@ export default class AuthenticationPage extends React.Component {

return (
<section className={styles.root}>
<Notifs CustomComponent={Toast} />
{loginError && <p>{loginError}</p>}
<Button
className={styles.button}
Expand Down
16 changes: 11 additions & 5 deletions src/backends/github/implementation.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export default class GitHub {

this.repo = config.getIn(["backend", "repo"], "");
this.branch = config.getIn(["backend", "branch"], "master");
this.api_root = config.getIn(["backend", "api_root"], "https://api.github.com");
this.token = '';
}

Expand All @@ -29,11 +30,16 @@ export default class GitHub {

authenticate(state) {
this.token = state.token;
this.api = new API({ token: this.token, branch: this.branch, repo: this.repo });
return this.api.user().then((user) => {
user.token = state.token;
return user;
});
this.api = new API({ token: this.token, branch: this.branch, repo: this.repo, api_root: this.api_root });
return this.api.user().then(user =>
this.api.isCollaborator(user.login).then((isCollab) => {
// Unauthorized user
if (!isCollab) throw new Error("Your GitHub user account does not have access to this repo.");
// Authorized user
user.token = state.token;
return user;
})
);
}

getToken() {
Expand Down