auth_username_field clobbering client-supplied filter

[bcattle]

There is a conflict when auth_username_field is defined and the client provides a filter affecting that same field. The issue is that auth_username_field adds a new filter criteria, e.g. {'username': 'bcattle'}. This criteria replaces any filter criteria supplied by the client (io/base.py line 188):

# if 'user-restricted resource access' is enabled and there's an Auth
# request active, add the username field to the query
username_field = config.DOMAIN[resource].get('auth_username_field')
if username_field and request.authorization and query is not None:
    query.update({username_field: request.authorization.username})

Hence if I was trying to look up /profiles/nicolaiarocci/ with auth_username_field='username' and my username==bcattle, internally Eve calls findOne() which returns /profiles/bcattle/ rather than failing.

On an update the request will then fail with HTTP 412, because the ETag I supplied for /profiles/nicolaiarocci/ fails to validate. This is the problem - the wrong error code is getting reported.

I added a special case to check for this conflict. Unfortunately the logic is convoluted. If auth_username_field=='_id' we also need to perform an ObjectId() comparison:

# if 'user-restricted resource access' is enabled and there's an Auth
# request active, add the username field to the query
username_field = config.DOMAIN[resource].get('auth_username_field')
if username_field and request.authorization and query is not None:
    # If the username_field *replaces* a field in the query,
    # and the two values are different, deny the request
    if username_field in query and \
       query[username_field] != request.authorization.username:
        # If the field is ID_FIELD, need additional check
        # to make sure the ObjectId is not equal :(
        if username_field != config.ID_FIELD or \
            query[username_field] != ObjectId(request.authorization.username):
            abort(401)
    else:
        query.update({username_field: request.authorization.username})

Does this logic seem reasonable?

This code may change with the fix proposed by @nicolaiarocci in #73 here. I'll write tests once that shakes out.

cc @rxl @asdine

[shea256]

Yeah nice catch @bcattle ! Like the update but it took me a little while to grasp what it was doing, so I re-wrote your code in the way that I personally thought it through logically. Would love to know what you think of this one.

My rationale for this slight layout modification: I find that the code is easier to break down logically if it first handles the standard condition and then considers the tricky conditions / exceptions (like when the username_field exists in the query and then when the field is an ObjectId).

# if 'user-restricted resource access' is enabled and there's an Auth
# request active, add the username field to the query
username_field = config.DOMAIN[resource].get('auth_username_field')
if username_field and request.authorization and query is not None:
    # if the username field is not in the query, update the query
    if username_field not in query:
        query.update({username_field: request.authorization.username})
    else:
        # if the username_field *replaces* a field in the query,
        # and the two values are different, deny the request
        req_auth_username = request.authorization.username if username_field \
            != config.ID_FIELD else ObjectId(request.authorization.username)
        if query[username_field] != req_auth_username:
            abort(401)

[bcattle]

Good call, I like the idea of handling the most common case first. But the ternary operator was confusing to me. How about this?

if username_field not in query:
    query.update({username_field: request.authorization.username})
else:           
    # If the username_field *replaces* a field in the query,
    # and the two values are different, deny the request
    if query[username_field] != request.authorization.username:
        # If `auth_username_field` is ID_FIELD,
        # also perform an ObjectId comparison
        if username_field != config.ID_FIELD or \
            query[username_field] != ObjectId(request.authorization.username):
            abort(401)

[shea256]

The part that confused me the most with your version was the sub-check. Your description says "If auth_username_field is ID_FIELD...", but your code describes something very different, which is more like "If auth_username_field is not ID_FIELD or ...". In order to mentally parse that line, you have to move from the first nonequivalence check down to the abort statement then up to the second check, etc.

How about this?

if username_field not in query:
    query.update({username_field: request.authorization.username})
else:           
    # If the username_field *replaces* a field in the query,
    # and the two values are different, deny the request
    if query[username_field] != request.authorization.username:
        abort(401)
        # If `auth_username_field` is ID_FIELD,
        # also perform an ObjectId comparison
    if username_field == config.ID_FIELD or \
        query[username_field] != ObjectId(request.authorization.username):
        abort(401)

Also, what are you using to make your code look different than mine?

[bcattle]

The only problem is that the if query[username_field] != request.authorization.username: will always fail if query[username_field] == ObjectId(request.authorization.username) and hence never get to the second if statement.

Prefix your code with three backticks and the name of the language to turn on syntax highlighting, see here.

[shea256]

Updated:

if username_field not in query:
    query.update({username_field: request.authorization.username})
else:           
    # If the username_field *replaces* a field in the query,
    # and the two values are different, deny the request
    if username_field == config.ID_FIELD and \
        query[username_field] != ObjectId(request.authorization.username):
        abort(401)
    elif query[username_field] != request.authorization.username:
        abort(401)

And thanks for the tip :)

[shea256]

Here's another option. IMO it's a little more elegant, but slightly more verbose. Also, it has a try except to catch the case when request.authorization.username isn't really a valid ObjectId.

def smart_compare(a, b):
    """ Compares a and b. A special case is handled where a is an ObjectId
        but b is a string representation of an ObjectId. """
    if isinstance(a, ObjectId):
        try:
            return a == ObjectId(b)
        except:
            return False
    else:
        return a == b

if username_field not in query:
    query.update({username_field: request.authorization.username})
else:           
    # If the username_field *replaces* a field in the query,
    # and the two values are different, deny the request
    if not smart_compare(query[username_field], request.authorization.username):
        abort(401)

[bcattle]

Interesting idea but we don't want to test for the validity of ObjectId at this point, because

1. Returning a 401 Unauthorized is not appropriate, a bad ObjectId string is a 400 Bad Request.
2. This check already happens elsewhere, in validation.py A string to ObjectId comparison function might make sense in this file.

Also you shouldn't do try / except without specifying an exception. Doing smart_compare(1, divide_by_zero(2)) will return False, swallowing an error and requiring you to get out the debugger.

[nicolaiarocci]

Good catch! We'll sort this out once #73 is fixed (soon enough - have it on a local branch already).

[nicolaiarocci]

@bcattle will you pull fc80fa8 and update this PR accordingly? Looks like the issue relevant, right?

[bcattle]

It's relevant if anyone would ever perform a filter on auth_field, which is now hard-coded to be an ObjectId. This seems like something we'd want to allow, so I'll update the branch.

[nicolaiarocci]

Not necessarily and ObjectId, but most of the time that's what will end up being used.

[shea256]

@bcattle good call

[bcattle]

Superceded by #104