Address issue #323 and enable GHE Auth #491

tortilaman · 2017-07-19T22:34:56Z

So far this is a basic fix for issue #323. Unauthorized users will not be able to get into the backend, and an alert window will popup informing them they need to speak with their site administrator to get access.

Once I figure out how to feed the error to AuthenticationPage.js I’ll make it look better.

Also, api_root wasn’t being fed to API.js, which was causing issues authenticating Github Enterprise users, since it was using the regular github api endpoint. I fed api_root in, which should enable Github Enterprise for everyone, and potentially other git providers?

erquhart · 2017-07-21T18:59:23Z

@tortilaman fyi I rebased your branch so that only the new commits are included in this PR.

erquhart

Just one small change, otherwise LGTM! Thanks!

erquhart · 2017-07-21T19:01:31Z

.eslintrc

@@ -11,5 +11,10 @@
  },
  "globals": {
    "CMS_ENV": true
+  },
+  "parserOptions": {


Remove this for now, can be brought up in a separate PR.

tortilaman · 2017-07-21T19:38:58Z

Yeah, I was gonna ask about rebasing. Figured that would be a good idea. I've actually now modified to provide better feedback so it uses the existing toast UI and redux-notifications. I feel comfortable merging at this point.

tortilaman · 2017-07-21T19:45:30Z

Apologies for the double commit there, was using github desktop instead of cli and sometimes it doesn't make it clear what it's doing.

erquhart · 2017-07-21T20:13:03Z

Fixed history again - if you do a hard reset to origin from your local repo, that should prevent further extraneous commits.

erquhart

Last thing, just updates to the error messages.

erquhart · 2017-07-21T20:14:14Z

src/backends/github/implementation.js

+          user.token = state.token;
+          return user;
+        }
+        throw new Error('No response from Github, that\'s odd.');


Replace with "GitHub is not responding, please try again."

That probably makes more sense, haha. Leftover from my initial testing

erquhart · 2017-07-21T20:16:04Z

src/backends/github/implementation.js

+      this.api.collaborator(user.login).then((status) => {
+        if (status === 404 || status === 403) {
+          // Unauthorized user
+          throw new Error("Sorry, you don't have CMS access.");


Replace with "Your GitHub user account does not have access to this repo."

I was thinking that it might be beneficial in terms of users' mental model to abstract it a bit from GitHub since I'm sure there's a lot of end-users doing editing and writing that might not necessarily be so familiar with GitHub.

Benaiah · 2017-07-21T20:36:13Z

src/backends/github/implementation.js

@@ -35,7 +35,11 @@ export default class GitHub {
      this.api.collaborator(user.login).then((status) => {
        if (status === 404 || status === 403) {
          // Unauthorized user
+<<<<<<< HEAD


Looks like you inadvertently committed a diff here.

tortilaman · 2017-07-21T21:32:46Z

Everything should be cleaned up now for a PR and the diff should've been addressed.

erquhart

LGTM - thanks!

Benaiah

Great changes, thanks for this! I have a couple nitpicks and a slightly bigger suggestion for refactoring the API code.

Benaiah · 2017-07-21T23:32:28Z

src/backends/github/AuthenticationPage.js

 import styles from './AuthenticationPage.css';

 export default class AuthenticationPage extends React.Component {
  static propTypes = {
-    onLogin: React.PropTypes.func.isRequired,
+    onLogin: React.PropTypes.func.isRequired


This is an unrelated change, and goes against our current eslint config.

Benaiah · 2017-07-21T23:33:25Z

src/backends/github/API.js

@@ -19,6 +19,10 @@ export default class API {
    return this.request("/user");
  }

+  collaborator(user) {


This method should be renamed and modified as per my refactoring suggestions below.

Benaiah · 2017-07-21T23:33:57Z

src/actions/auth.js

@@ -60,6 +63,11 @@ export function loginUser(credentials) {
        dispatch(authenticate(user));
      })
      .catch((error) => {
+        dispatch(notifSend({


👍, this is a long-awaited change.

Benaiah · 2017-07-21T23:38:11Z

.gitignore

@@ -6,3 +6,6 @@ npm-debug.log
 .tern-project
 yarn-error.log
 .vscode/
+manifest.yml
+.imdone/
+config.local.yml


Don't know how I feel about these changes - on one hand, the .gitignore isn't super important; OTOH, I'd rather keep it small as it can cause confusion if it inadvertently ends up ignoring paths that we end up using, and if we add ignores for each contributor's individual tooling (rather than those that every contributor needs to ignore, such as build artifacts), I can see the .gitignore becoming too large pretty quickly. Additionally, these changes show up in history and diffs despite being unrelated to the actual purpose of the PR. @erquhart thoughts?

I personally think it's fine. manifest.yml is a Bluemix artifact, and .imdone is from Atom. A lengthy .gitignore isn't a bad thing if it improves the developer experience for contributors.

@tortilaman what is config.local.yml from?

config.local.yml is storing my backend config so I don't have to go searching for it to test with GitHub.

@tortilaman let's leave that value out since it's not a standard file, but the other two can stay.

@erquhart sounds like a solid policy to me, thanks for the clarification.

Benaiah · 2017-07-21T23:43:01Z

src/actions/auth.js

@@ -1,4 +1,7 @@
 import { currentBackend } from '../backends/backend';
+import { actions as notifActions } from 'redux-notifications';
+
+const { notifSend } = notifActions;


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).

Benaiah · 2017-07-21T23:43:29Z

src/backends/github/API.js

-      if (contentType && contentType.match(/json/)) {
+      if (url.indexOf('/collaborators/') >= 0) {
+        return responseStatus;
+      } else if (contentType && contentType.match(/json/)) {


While I understand the motivation behind this code, this is bound to cause bugs in the future by special-casing a particular URL in what is otherwise a generic request function. This should really be done in collaborator. One issue I see with the response method as it stands is that JSON responses reject the promise when an error status is present, but text responses do not.

@erquhart if you agree that that should be fixed, I can include it in my GH API refactor branch I'm preparing to PR (the branch name is gh-api-refactors, there's a little there already).

@tortilaman for now, you should be able to expect a JSON response from the GH API. Thus, any non-200 statuses will cause the promise to reject with the APIError in the catch block just below these lines. In addition, My suggestions for refactoring this are as follows:

Remove this logic from request.

Rename the collaborator method to isCollaborator, and have that return a promise that either rejects or resolves to a boolean, representing whether the user is a collaborator or not.

Catch the 404, 403, and 204 statuses in isCollaborator. If the status is one of those three, resolve the promise to the appropriate boolean. Otherwise, rethrow the error. (remember, JSON responses reject the error if the status is not 200-99, so you don't need any additional

Remove the now-unnecessary status code checks from authenticate and change them to handle the boolean that isCollaborator is returning. Add a catch block to handle unexpected errors, and move the throw new Error('GitHub is not responding, please try again.'); to the catch block.

I'd be happy to elaborate on this further if necessary, and I can always refactor this myself if you'd rather move on, but either way I'd like this to be refactored before a merge.

Great breakdown @Benaiah, I'm on board with this.

@Benaiah Would isCollaborator then need to follow promise syntax and utilize resolve() etc.? I'm of the understanding that without doing that .then((response)... will still work based on what I have now.

Benaiah · 2017-07-21T23:44:43Z

src/backends/github/implementation.js

+          // Authorized user
+          user.token = state.token;
+          return user;
+        }


The preceding status-checking logic should move to collaborator/isCollaborator (whatever that ends up being named) instead of being handled here - the only part of this that's relevant to authentication is whether the user is a collaborator or not, not the specific status code. This is part of the more detailed refactor I suggested above.

erquhart · 2017-07-22T02:30:49Z

@Benaiah leaving this to you, merge once you're happy with the changes.

erquhart · 2017-07-24T15:30:38Z

@tortilaman merge conflict.

# Conflicts: # src/backends/github/API.js

erquhart · 2017-07-24T17:48:49Z

@Benaiah also, we should squash-merge.

tortilaman · 2017-07-24T18:15:32Z

Apologies, I was definitely going to squash this on my last commit, but the interactive rebase got quite complicated with the merge and figured I would have to re-order commits and such, and I just figured it would be better left to you.

erquhart · 2017-07-24T18:50:03Z

No problem! Squash merging is a simple solution, no sweat.

tortilaman · 2017-07-31T18:38:35Z

Just pulled master into this and some issues seem to have popped up. I'll update when I think it should be ready to merge again.

…nd-fix

erquhart · 2017-08-01T14:31:35Z

@tortilaman sounds good, reach out if you need help.

Change to check for generic push access instead of contributor status

tortilaman · 2017-08-01T18:24:26Z

Alright @erquhart @Benaiah I think this is ready for a squash merge.

Benaiah

LGTM with one small change.

Benaiah · 2017-08-01T19:54:44Z

src/backends/github/API.js

@@ -67,6 +80,9 @@ export default class API {
      if (contentType && contentType.match(/json/)) {
        return this.parseJsonResponse(response);
      }
+      if (responseStatus !== 200) {


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)) {

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

tortilaman · 2017-08-01T20:33:15Z

Alright, everything resolved.

Benaiah · 2017-08-02T03:28:55Z

@tortilaman thanks so much!

tortilaman · 2017-08-02T18:14:42Z

Thanks for all the feedback!

`isCollaborator` was created in #491 to block login if a user did not have write (push) permissions to a repo, by going through the list of a users repos until it found the right one. It did not institute pagination, however, so if a user had enough repos that the one in question was on another page, the CMS would assume that they did not have permission and block the login. This commit fixes the problem by calling the API for the specific repo instead of getting the whole list.

erquhart force-pushed the master branch from 7c59cdd to 02afe17 Compare July 21, 2017 18:58

erquhart suggested changes Jul 21, 2017

View reviewed changes

erquhart force-pushed the master branch from 3b7fe50 to 0b67e85 Compare July 21, 2017 20:12

erquhart suggested changes Jul 21, 2017

View reviewed changes

Benaiah reviewed Jul 21, 2017

View reviewed changes

tortilaman force-pushed the master branch from c6fe398 to 048fb04 Compare July 21, 2017 21:00

Prevent unauthorized CMS access & Enable GHE usage

de04389

tortilaman force-pushed the master branch from 048fb04 to de04389 Compare July 21, 2017 21:30

erquhart approved these changes Jul 21, 2017

View reviewed changes

Benaiah suggested changes Jul 21, 2017

View reviewed changes

Refactor of new 'isCollaborator' GitHub API method

2a89f3e

tortilaman force-pushed the master branch from 4888acc to 2a89f3e Compare July 24, 2017 16:26

tortilaman added 2 commits July 24, 2017 09:27

Merge remote-tracking branch 'netlify/master'

7d9b14c

# Conflicts: # src/backends/github/API.js

fixed conflicts

049ef33

tortilaman added 2 commits July 31, 2017 11:42

Merge remote-tracking branch 'netlify/master'

d9b5edf

Merge remote-tracking branch 'netlify/master' into unauthorized-backe…

ea32e9c

…nd-fix

tortilaman added 2 commits August 1, 2017 11:15

Fix to check for general repo push access instead of "contributor"

06cb4ca

Merge branch 'unauthorized-backend-fix'

8937c7d

Change to check for generic push access instead of contributor status

Update to .gitignore

f2f8b26

Benaiah suggested changes Aug 1, 2017

View reviewed changes

Remove some outdated checks

9ef3d05

Benaiah approved these changes Aug 2, 2017

View reviewed changes

Benaiah merged commit 6805a69 into decaporg:master Aug 2, 2017

tech4him1 mentioned this pull request Aug 19, 2017

Fix denied login for users with many repos. #543

Merged

tech4him1 mentioned this pull request Aug 28, 2017

Check user write access even with remembered credentials #556

Closed

Address issue #323 and enable GHE Auth #491

Address issue #323 and enable GHE Auth #491

Conversation

tortilaman commented Jul 19, 2017

erquhart commented Jul 21, 2017

erquhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tortilaman commented Jul 21, 2017

tortilaman commented Jul 21, 2017

erquhart commented Jul 21, 2017

erquhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tortilaman commented Jul 21, 2017

erquhart left a comment

Choose a reason for hiding this comment

Benaiah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erquhart commented Jul 22, 2017

erquhart commented Jul 24, 2017

erquhart commented Jul 24, 2017

tortilaman commented Jul 24, 2017

erquhart commented Jul 24, 2017

tortilaman commented Jul 31, 2017 • edited Loading

erquhart commented Aug 1, 2017 • edited Loading

tortilaman commented Aug 1, 2017

Benaiah left a comment

Choose a reason for hiding this comment

Benaiah Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

tortilaman Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

tortilaman commented Aug 1, 2017

Benaiah commented Aug 2, 2017

tortilaman commented Aug 2, 2017

tortilaman commented Jul 31, 2017 •

edited

Loading

erquhart commented Aug 1, 2017 •

edited

Loading

Benaiah Aug 1, 2017 •

edited

Loading

tortilaman Aug 1, 2017 •

edited

Loading