nicolaiarocci/eve — pull request #104 by bcattle (2013-09-12 23:40:58+00:00)
Title: Cleanup and hardening of User-Restricted Resource Access

#### Cleanup and hardening of User-Restricted Resource Access

This supercedes and replaces #77. It is a rewrite of that code taking into account the changes introduced in #73.

##### The problem

The condition specified by `auth_field` conflicts with the condition specified by any other filter operation like
-	A URL of the form `/users/abcdef01234567890abcdef0/`
-	An [advanced datasource pattern](http://python-eve.org/config.html#advanced-datasource-patterns) of the form

``` python
people = {
'datasource': {
'filter': {'username': {'$exists': True}}
}
}
```

-	a `?where={}` query param in the URL

Right now the `auth_field` condition either transparently replaces the requested document with the one authorized by `auth_field` or returns an empty set. The correct behavior would be to return HTTP 401 if the user is requesting something prohibited by `auth_field` or an empty set if the request is allowed but there are no matching documents.

##### The solution

I implemented logic to this effect in [eve/io/base.py](https://github.com/bcattle/eve/blob/auth_field_clobbering_filter/eve/io/base.py#L246) starting at line 246:

``` python
# We need to run the 'user-restricted resource access' check
auth_field = resource_dict.get('auth_field', None)
if auth_field and request.authorization \
and query is not None:
# If the auth_field *replaces* a field in the query,
# and the values are /different/, deny the request
# This prevents the auth_field condition from
# overwriting the query (issue #77)
curr_req_auth_value = getattr(self.app.auth, auth_field)
auth_field_in_query = \
self.app.data.query_contains_field(query,
auth_field)
if auth_field_in_query and \
self.app.data.get_value_from_query(
query, auth_field) != curr_req_auth_value:
abort(401, description=debug_error_message(
'Incompatible User-Restricted Resource request. '
'Request was for <%s>=%s but `auth_field` '
'requires <%s>=%s.' %
(auth_field, auth_field_in_query, auth_field,
curr_req_auth_value)
))
else:
query = self.app.data.combine_queries(
query, {auth_field: curr_req_auth_value}
)
```
Notice the use of the new method [`combine_queries()`](https://github.com/bcattle/eve/blob/972b73081bee7d0c4352a1117b5369e6280065e9/eve/io/mongo/mongo.py#L246). This replaces the `query.update({new_field: new_value})` operation that we had. To see how this works, imagine we had a query like

``` python
>>> query = {'username': {'$exists': False}}
```

Old operation:

``` python
>>> query.update({'username': 'mike'})
{'username': 'mike'} # bad, information was lost
```

New operation:

``` python
>>> self.app.data.combine_queries(query, {'username': 'mike'})
{'$and': [{'username': {'$exists': False}}, {'username': 'mike'}]} # no information loss
```
Of course this particular query doesn't make sense, but it illustrates the principle.

This is a significant change, because it means we can no longer do `query.update`, get a field with `query[field]`, or test for membership with ` field in query`

I replaced these operations with helper methods:
- [`get_value_from_query()`](https://github.com/bcattle/eve/blob/972b73081bee7d0c4352a1117b5369e6280065e9/eve/io/mongo/mongo.py#L285)
- [`query_contains_field()`](https://github.com/bcattle/eve/blob/972b73081bee7d0c4352a1117b5369e6280065e9/eve/io/mongo/mongo.py#L311)

There is also a helper method [`_convert_query_objectids()`](https://github.com/bcattle/eve/blob/972b73081bee7d0c4352a1117b5369e6280065e9/eve/io/mongo/mongo.py#L383) that handles scanning both regular and compound queries and converting instances of `ID_FIELD` to `ObjectId(...)`

##### Changes:

1.	Tests load settings file from an absolute path, allows the use of nose – e.g. `nosetests eve.tests.auth`
2.	Added functions to `eve.io.mongo`: `combine_queries()`, `get_value_from_query()`, and `query_contains_field()`
3.	Added abstract versions of the above to `eve.io.base`
4.	Added a function to `eve.io.mongo`: `_convert_query_objectids()`. This addresses string <--> ObjectId conversion in a new "compound query"
5.	Added logic to `eve.io.base` that performs the actual User-Restricted Resource Access check
6. Added more tests, including tests for both positive and negative behavior with `auth_field`

##### Issues:

3. Would it be more appropriate to factor out query helper methods into a new `MongoQuery` class? This could also provide transparent support for ObjectId <--> string comparison
1.	ObjectId handling continues to be an annoyance. At some points the app expects ids to be strings, and other places it expects `ObjectId` objects. This potentially breaks encapsulation, as `ObjectId`s are a MongoDB-specific feature. This PR punts on this issue, assuming that all ids that reach [line 256](https://github.com/bcattle/eve/blob/972b73081bee7d0c4352a1117b5369e6280065e9/eve/io/base.py#L256) of base.py have already been converted to `ObjectId`.
2. Due to the above, **any user-specified auth class must set a member variable referencing `ID_FIELD` to be an `ObjectId` _not a string_!**

You need to do this

``` python
class ValidBasicAuth(BasicAuth):
def check_auth(self, username, password, allowed_roles, resource, method):
# ...
if is_valid:
# This MUST BE AN OBJECTID, not a string.
self._id = ObjectId('deadbeefdeadbeefdeadbeef')
```

Not this

``` python
class ValidBasicAuth(BasicAuth):
def check_auth(self, username, password, allowed_roles, resource, method):
# ...
if is_valid:
self._id = 'deadbeefdeadbeefdeadbeef' # wrong
```

The docs should probably say this and we should probably add an assertion to enforce this. If people agree I'll open a separate issue to take care of it.