Skip to content
This repository has been archived by the owner on May 4, 2023. It is now read-only.

Pass-through authentication #150

Merged
merged 0 commits into from Feb 20, 2012
Merged

Pass-through authentication #150

merged 0 commits into from Feb 20, 2012

Conversation

jalpedersen
Copy link

I know that you have previously mentioned that you would prefer to implement this sort of thing using a UserRealm, but then one would have to maintain a session in Jetty just for the sake of CouchDB.

One great benefit of going the pass-through way is that it makes it much easier to implement secure search when dealing with multiple separate CouchApps (as I pass both the CouchDB auth-session cookie as well as the authentication header if present).
Yet another benefit is that one does not have to have to store a couchdb user's username and password in clear-text in couchdb-lucene.ini.

Note that this commit will not break the current behaviour. If credentials are specified in couchdb-lucene.ini, these will still be used, so any user can access what the credentials give access to.

@rnewson
Copy link
Owner

rnewson commented Feb 19, 2012

Hi. thanks for the patch, unfortunately I have a couple of issues with it.

  1. The Couch class is being coupled to the context of a single request. This only works because a new Couch object is created for each request, but that's only because they are lightweight (the httpclient object holds heavier things like the connection pool).
  2. Using the credentials of the user, instead of an admin, may prevent the creation of the _local/lucene document (which is where the UUID is stored), if that user has only read access.
  3. The code that detects and copies authentication credentials looks quite fragile, it would fail for oauth requests, etc.
  4. The patch introduces whitespace changes.

I understand the reticence on storing plaintext passwords, perhaps that's worth remedying directly? couchdb-lucene.ini needs only to be readable/writable by the user the couchdb-lucene process runs as. Would there be any improvement if the passwords were held in a separate file with different permissions?

@jalpedersen
Copy link
Author

  1. I saw the Couch class as a request-scoped thing. Where would you prefer to keep request information?
  2. I missed that part. But I think the bigger problem is that any user can access all databases including those for which he has no read or write permissions. Perhaps one should consider only using the admin credentials when adding the _local/lucene document?
  3. Yes, I'll give you that. Perhaps one should just pass on all headers instead of trying to be clever? The overhead should be minimal.
  4. Sorry about that.

I think my biggest beef is that all users has access to all databases, not so much that the credentials are stored in plain text.

@rnewson
Copy link
Owner

rnewson commented Feb 19, 2012

Ah, I get you. CouchDB-Lucene assumes that it's being called from couchdb, via either the old or new externals API. I'd definitely like to improve that.

A hybrid approach does make sense. C-L could use the credentials from couchdb-lucene.ini only in the case that it's trying to create the _local/lucene doc (I'm pretty sure that's the only write operation it ever makes), and simply echoes back the authentication headers in all other cases.

As for how to pass those headers, I'd either extend each method to add a Credentials parameter (for example), or a wrapper around HttpClient that makes it explicit that it carries per-request authentication data (AuthenticatingHttpClient, for example).

The main problem I see is that the DatabaseIndexer, that reads source databases and updates target Lucene indexes, is persistent (it starts on first request and continues until it crashes), it performs work for many users. It, too, would need to use the admin credentials.

Another suggestion would be to make a GET request to _session using the authentication headers (i.e, the Authorization and/or Cookie headers) and use the result to determine access levels. If it returns 401, then don't allow the search to proceed. Everything else stays the same.

@jalpedersen jalpedersen merged commit daf007b into rnewson:master Feb 20, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants