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

Oauth support #48

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Oauth support #48

wants to merge 5 commits into from

Conversation

raphaelhoffmann
Copy link

@netj Added basic support for authentication and authorization. If you have time, you might want to look if you want to integrate this into Mindbender.

@raphaelhoffmann
Copy link
Author

See the following instructions for how to use this module.

https://github.com/HazyResearch/mindbender/blob/oauth-support/auth/README.md

@netj
Copy link
Contributor

netj commented Sep 21, 2015

First of all, thanks for sharing this high-quality code. I also want auth support in mindbender, but some parts of this PR feel like an overkill. Here are my questions:

  1. Does authorization only check URL /? Shouldn't it be a middleware that gatekeeps all paths (except the one for authentication)?
  2. Why do we need to rely on Google IDs? Can't we support simple user-password pairs first, then also OAuth or OpenID? One implicit but important design point of mindbender has been the ability to use it offline (or off the grid). Relying only on OAuth compromises that, and moreover it seems to be complicating the setup too much.
  3. Why is mongodb used for managing authorizations? Is it used for anything else? If not, can't we use a simple JSON file that holds a list of known users (with password hashes or OAuth ids)? Again, adding another moving part seems too much for just managing an ACL. I'm not sure if there's a use case with >10k users, but even then the file approach would probably work unless the ACL constantly changes.

@raphaelhoffmann
Copy link
Author

You're right that this could be improved in several ways. It's the quickest thing we could come up with, and I thought I'd separate it out from the other changes we're making in case you are interested in picking it up and enhancing it. There are definitely a few things you might want to change.

  1. Yes. It should check other paths as well, and could be called as some sort of middleware to avoid calls to ensureAuthenticated in submodules.
  2. Right, a simple username/passwords strategy and other OAuth providers would be interesting as well. At this moment, we only need OAuth with Google, so we built the MVP :).
  3. JSON files instead of mongo is fine too. I would also add that the authentication works quite well, but there might be a more elegant approach to authorization (I couldn't find a good example for that).

@chrismre
Copy link

Is it difficult to simply have a switch in the config file for oauth?

Chris

On Sun, Sep 20, 2015 at 6:07 PM Raphael Hoffmann notifications@github.com
wrote:

You're right that this could be improved in several ways. It's the
quickest thing we could come up with, and I thought I'd separate it out
from the other changes we're making in case you are interested in picking
it up and enhancing it. There are definitely a few things you might want to
change.

  1. Yes. It should check other paths as well, and could be called as
    some sort of middleware to avoid calls to ensureAuthenticated in submodules.
  2. Right, a simple username/passwords strategy and other OAuth
    providers would be interesting as well. At this moment, we only need OAuth
    with Google, so we built the MVP :).
  3. JSON files instead of mongo is fine too. I would also add that the
    authentication works quite well, but there might be a more elegant approach
    to authorization (I couldn't find a good example for that).


Reply to this email directly or view it on GitHub
#48 (comment)
.

@alldefector
Copy link
Contributor

Just to second @netj and @chrismre,

  1. We shouldn't hard code the auth params in the compiled auth-api.coffee. It should be configurable out of the big final binary. Env vars would be an option, but maybe this is a good reason to introduce a config file.
  2. Mongo is probably too heavy. Can we use an embedded DB instead (SQLite or any embedded doc/KV store)?

@alldefector
Copy link
Contributor

Confirmed: anonymous users can bypass this middleware and successfully query the ES proxy (even if the request has not csrftoken cookie). I'm not familiar with nodejs enough to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants