HazyResearch / mindbender

Tools for iterative knowledge base development with DeepDive
116 stars 32 forks source link

Oauth support #48

Open raphaelhoffmann opened 8 years ago

raphaelhoffmann commented 8 years ago

@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 commented 8 years ago

See the following instructions for how to use this module.

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

netj commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 https://github.com/HazyResearch/mindbender/pull/48#issuecomment-141852168 .

alldefector commented 8 years ago

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 commented 8 years ago

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.