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

Don't allow non repo members into admin #323

Closed
chadfawcett opened this issue Mar 24, 2017 · 12 comments
Closed

Don't allow non repo members into admin #323

chadfawcett opened this issue Mar 24, 2017 · 12 comments

Comments

@chadfawcett
Copy link

- Do you want to request a feature or report a bug?

  • bug

- What is the current behaviour?

  • If I log in as a GitHub user that is not a member of the repository, I am able to see part of the admin interface. I am even able to go to the collection editing page and fill out the form. I get an error when I attempt to save the new collection entry, but it seems odd that anyone could log into my admin and have limited access.

screen shot 2017-03-23 at 9 38 27 pm

screen shot 2017-03-23 at 9 39 17 pm

- If the current behaviour is a bug, please provide the steps to reproduce.

  • Log in as a GitHub user that is not a member of the repository

- What is the expected behaviour?

  • I would expect that a user would get an error message saying they are not a member of the repository, and then not aloud to see the admin interface.

- Please mention your node.js, and operating system version.

  • Node: v6.10.1
  • macOS Sierra
@chadfawcett
Copy link
Author

Ultimately, the interface is only showing information that is publicly available in the config.yml, so it's not a security concern. However, I think it would be more user friendly to display an error message instead of a non-functioning UI.

@tech4him1
Copy link
Contributor

GitHub Permission Checking APIs: https://developer.github.com/v3/repos/collaborators/

@colindensem
Copy link

colindensem commented Apr 10, 2017

I was just coming to ask about this. Actually found out by mistake as a collaborator hadn't joined properly. So he was seeing API errors and I was off down a rabbit hole till I saw this.

@erquhart
Copy link
Contributor

It's a priority for sure. I don't expect it would be a very heavy lift if anyone wants to take a crack at it.

@tech4him1
Copy link
Contributor

Not very familiar with React -- would you put this in AuthenticationPage.js or API.js?

@josephearl
Copy link
Contributor

josephearl commented Apr 12, 2017

Does the same issue apply to the netlify-auth backend?

If it's just GitHub, I think the best place to put it would be in Authenticator in lib/netlify-auth.js, and callback with an error if the user cannot write to the repo.

@tech4him1
Copy link
Contributor

tech4him1 commented Apr 12, 2017

Good point @josephearl, my only thought was that since we may add GitLab support eventually, we might want to keep the implementations separated.

@josephearl
Copy link
Contributor

josephearl commented Apr 12, 2017

Yes, the same fix might need to be applied twice. The github backend uses Authenticator from lib/netlify-auth.js. The netlify-auth backend uses NetlifyAuthClient from netlify-auth-js (an npm package).

@erquhart erquhart added this to the 1.0 milestone Jun 28, 2017
@t1merickson
Copy link

Need design for denied screen; when a user tries to access/login but they do not have push access to the repo

@tortilaman
Copy link
Contributor

Would it make sense to also add some kind of .htaccess or or other permissions solution to deny the general public access to the config.yml file?

I'm gonna try and figure out an implementation for both of these asap as I'd like to make sure this is all taken care of before I deploy anything to production.

@erquhart
Copy link
Contributor

Actually, @Benaiah is working on #341, which should result in the config file only containing backend details (which must be publicly accessible), while the remainder of the config would be fetched from the repo. So public access of config.yml shouldn't be a problem.

Benaiah pushed a commit that referenced this issue Aug 2, 2017
…prise (#491)

* Prevent unauthorized CMS access and enable use of GitHub Enterprise
@tortilaman
Copy link
Contributor

Unless anyone has any issues, I'm going to close this.

@erquhart erquhart modified the milestones: 0.5.0, 1.0 Aug 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants