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.

Sign up for GitHub

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

Jump to bottom

Fix PUT behavior with User-Restricted Resource Access #1130

Merged
merged 2 commits into from
Apr 3, 2018
Merged

Fix PUT behavior with User-Restricted Resource Access #1130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7adbe7
Fix PUT behavior with User-Restricted Resource Access
lmoretto Mar 30, 2018
4ceac4d
Reduced error description details
lmoretto Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 36 additions & 27 deletions eve/io/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ def aggregate(self, resource, pipeline, options):
"""
raise NotImplementedError

def find_one(self, resource, req, **lookup):
def find_one(self, resource, req, check_auth_value=True,
force_auth_field_projection=False, **lookup):
""" Retrieves a single document/record. Consumed when a request hits an
item endpoint (`/people/id/`).

Expand All @@ -164,6 +165,14 @@ def find_one(self, resource, req, **lookup):
etc). As we are going to only look for one document here,
the only req attribute that you want to process here is
``req.projection``.
:param check_auth_value: a boolean flag indicating if the find
operation should consider user-restricted
resource access. Defaults to ``True``.
:param force_auth_field_projection: a boolean flag indicating if the
find operation should always
include the user-restricted
resource access field (if
configured). Defaults to ``False``.

:param **lookup: the lookup fields. This will most likely be a record
id or, if alternate lookup is supported by the API,
Expand Down Expand Up @@ -343,7 +352,8 @@ def datasource(self, resource):
return source, filter_, projection, sort,

def _datasource_ex(self, resource, query=None, client_projection=None,
client_sort=None):
client_sort=None, check_auth_value=True,
force_auth_field_projection=False):
""" Returns both db collection and exact query (base filter included)
to which an API resource refers to.

Expand Down Expand Up @@ -446,35 +456,34 @@ def _datasource_ex(self, resource, query=None, client_projection=None,

# Only inject the auth_field in the query when not creating new
# documents.
if request and request.method not in ('POST', 'PUT'):
if request and request.method != 'POST' and (
check_auth_value or force_auth_field_projection
):
auth_field, request_auth_value = auth_field_and_value(resource)
if auth_field and request_auth_value:
if query:
# 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)
auth_field_in_query = \
self.app.data.query_contains_field(query, auth_field)
if auth_field_in_query and \
if auth_field:
if request_auth_value and check_auth_value:
if query:
# 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)
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) != request_auth_value:
abort(401, description='Incompatible User-Restricted '
'Resource request. '
'Request was for "%s"="%s" but `auth_field` '
'requires "%s"="%s".' % (
auth_field,
self.app.data.get_value_from_query(
query, auth_field),
auth_field,
request_auth_value)
)
desc = 'Incompatible User-Restricted Resource ' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might disclose a little too much info to the client. Yes, the desc field is only emitted in debug mode, but you never know someone might leave debug on by mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description string was already present, I didn't change it. I simply restructured this piece of code to fix a Flake8 line too long error. Do you want me to change the description anyway? What should I write?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, it was there already. Let us obfuscate it a little bit, maybe by simply returning the first part of the message (drop the "request was for but xx was excpected")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done ;)

'request.'
abort(401, description=desc)
else:
query = self.app.data.combine_queries(
query, {auth_field: request_auth_value}
)
else:
query = self.app.data.combine_queries(
query, {auth_field: request_auth_value}
)
else:
query = {auth_field: request_auth_value}
query = {auth_field: request_auth_value}
if force_auth_field_projection:
fields[auth_field] = 1
return datasource, query, fields, sort

def _client_projection(self, req):
Expand Down
7 changes: 5 additions & 2 deletions eve/io/mongo/mongo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,8 @@ def find(self, resource, req, sub_resource_lookup):

return self.pymongo(resource).db[datasource].find(**args)

def find_one(self, resource, req, **lookup):
def find_one(self, resource, req, check_auth_value=True,
force_auth_field_projection=False, **lookup):
""" Retrieves a single document.

:param resource: resource name.
Expand Down Expand Up @@ -312,7 +313,9 @@ def find_one(self, resource, req, **lookup):
datasource, filter_, projection, _ = self._datasource_ex(
resource,
lookup,
client_projection)
client_projection,
check_auth_value=check_auth_value,
force_auth_field_projection=force_auth_field_projection)

if (config.DOMAIN[resource]['soft_delete']) and \
(not req or not req.show_deleted) and \
Expand Down
16 changes: 14 additions & 2 deletions eve/methods/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@
from backport_collections import Counter


def get_document(resource, concurrency_check, original=None, **lookup):
def get_document(resource, concurrency_check, original=None,
check_auth_value=True, force_auth_field_projection=False,
**lookup):
""" Retrieves and return a single document. Since this function is used by
the editing methods (PUT, PATCH, DELETE), we make sure that the client
request references the current representation of the document before
Expand All @@ -45,6 +47,14 @@ def get_document(resource, concurrency_check, original=None, **lookup):
:param resource: the name of the resource to which the document belongs to.
:param concurrency_check: boolean check for concurrency control
:param original: in case the document was already retrieved before
:param check_auth_value: a boolean flag indicating if the find operation
should consider user-restricted resource
access. Defaults to ``True``.
:param force_auth_field_projection: a boolean flag indicating if the
find operation should always include
the user-restricted resource access
field (if configured). Defaults to
``False``.
:param **lookup: document lookup query

.. versionchanged:: 0.6
Expand All @@ -70,7 +80,9 @@ def get_document(resource, concurrency_check, original=None, **lookup):
if original:
document = original
else:
document = app.data.find_one(resource, req, **lookup)
document = app.data.find_one(resource, req, check_auth_value,
force_auth_field_projection,
**lookup)

if document:
e_if_m = config.ENFORCE_IF_MATCH
Expand Down
16 changes: 14 additions & 2 deletions eve/methods/put.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from flask import current_app as app, abort
from werkzeug import exceptions

from eve.auth import requires_auth
from eve.auth import auth_field_and_value, requires_auth
from eve.methods.common import get_document, parse, payload as payload_, \
ratelimit, pre_event, store_media_files, resolve_user_restricted_access, \
resolve_embedded_fields, build_response_document, marshal_write_response, \
Expand Down Expand Up @@ -113,7 +113,13 @@ def put_internal(resource, payload=None, concurrency_check=False,
if payload is None:
payload = payload_()

original = get_document(resource, concurrency_check, **lookup)
# Retrieve the original document without checking user-restricted access,
# but returning the document owner in the projection. This allows us to
# prevent PUT if the document exists, but is owned by a different user
# than the currently authenticated one.
original = get_document(resource, concurrency_check,
check_auth_value=False,
force_auth_field_projection=True, **lookup)
if not original:
if config.UPSERT_ON_PUT:
id = lookup[resource_def['id_field']]
Expand All @@ -126,6 +132,12 @@ def put_internal(resource, payload=None, concurrency_check=False,
else:
abort(404)

# If the document exists, but is owned by someone else, return
# 403 Forbidden
auth_field, request_auth_value = auth_field_and_value(resource)
if auth_field and original.get(auth_field) != request_auth_value:
abort(403)

last_modified = None
etag = None
issues = {}
Expand Down
40 changes: 40 additions & 0 deletions eve/tests/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,13 +643,33 @@ def test_put(self):
headers=headers,
content_type='application/json'))
self.assert200(status)
etag = '"%s"' % response['_etag']

# document still accessible with same auth
data, status = self.parse_response(
self.test_client.get(url, headers=self.valid_auth))
self.assert200(status)
self.assertEqual(data['ref'], new_ref)

# put on same item with different auth fails
original_auth_val = self.resource['authentication'].request_auth_value
self.resource['authentication'].request_auth_value = 'alt'
alt_auth = ('Authorization', 'Basic YWx0OnNlY3JldA==')
alt_changes = {"ref": "1111111111111111111111111"}
headers = [('If-Match', etag), alt_auth]
response, status = self.parse_response(
self.test_client.put(url, data=json.dumps(alt_changes),
headers=headers,
content_type='application/json'))
self.assert403(status)

# document still accessible with original auth
self.resource['authentication'].request_auth_value = original_auth_val
data, status = self.parse_response(
self.test_client.get(url, headers=self.valid_auth))
self.assert200(status)
self.assertEqual(data['ref'], new_ref)

def test_put_resource_auth(self):
# no global auth.
self.app = Eve(settings=self.settings_file)
Expand Down Expand Up @@ -680,13 +700,33 @@ def test_put_resource_auth(self):
headers=headers,
content_type='application/json'))
self.assert200(status)
etag = '"%s"' % response['_etag']

# document still accessible with same auth
data, status = self.parse_response(
self.app.test_client().get(url, headers=self.valid_auth))
self.assert200(status)
self.assertEqual(data['ref'], new_ref)

# put on same item with different auth fails
original_auth_val = resource_def['authentication'].request_auth_value
resource_def['authentication'].request_auth_value = 'alt'
alt_auth = ('Authorization', 'Basic YWx0OnNlY3JldA==')
alt_changes = {"ref": "1111111111111111111111111"}
headers = [('If-Match', etag), alt_auth]
response, status = self.parse_response(
self.app.test_client().put(url, data=json.dumps(alt_changes),
headers=headers,
content_type='application/json'))
self.assert403(status)

# document still accessible with original auth
resource_def['authentication'].request_auth_value = original_auth_val
data, status = self.parse_response(
self.app.test_client().get(url, headers=self.valid_auth))
self.assert200(status)
self.assertEqual(data['ref'], new_ref)

def test_put_bandwidth_saver_off_resource_auth(self):
""" Test that when BANDWIDTH_SAVER is turned off the auth_field is
not exposed in the response payload
Expand Down