Arvados

reviewing 2d6884f5b20c349e7fd28a51cd876d40524186ad

small changes: services/api/db/structure.sql has a extra space after "Tablespace:"

I eyeball it the code and so far nothing pops ups as terrible

I'm running the tests now

Reviewing afe4660. All the new code looks good. I'm running tests now, but everything's passed so far. At most, I have one question about it. I also have some questions about some existing code and how it relates to these change. If you think these are out of scope, that's totally fair, but I wanted to raise these in the hopes of avoiding another go-around on this ticket if we can help it.

In particular, there seems to be imperfect overlap in the security measures implemented by find_objects_for_index and current_api_client_is_trusted. Both seem to be aiming toward the same basic goal, but there are discrepancies in what they enforce, and how.

• find_objects_for_index scrubs @filters so it only includes exact matches on uuid and api_token.
• It also scrubs @where, but it allows any search on uuid. (This seems like a bug.)
• If I'm reading [['uuid'], ['api_token']].include? filters right, current_api_client_is_trusted returns an error to the client if it tries to filter on any other attribute. It doesn't check the operator.

This seems especially confusing because what happens depends on the order these filters run in. I believe find_objects_for_index runs first, since it's declared first. If that's right, I believe the condition in current_api_client_is_trusted is a noop, because find_objects_for_index will have already scrubbed @filters by the time that check runs.

I think I personally would advocate that we should commit to a single security strategy—either scrub filters, or fully reject requests with unallowed filters—and we should enforce it in one place. Having a little of both seems like an opportunity for corner cases to fall through the cracks.

If you decide all that's out of scope, my one question: would if [['uuid'], ['api_token']].include? filters be better written if (filters - %w(uuid api_token)).empty? ? It should be allowed to filter on both attributes simultaneously, right?

Brett Smith wrote:

Reviewing afe4660. All the new code looks good. I'm running tests now, but everything's passed so far. At most, I have one question about it. I also have some questions about some existing code and how it relates to these change. If you think these are out of scope, that's totally fair, but I wanted to raise these in the hopes of avoiding another go-around on this ticket if we can help it.

I agree this area seems more complicated than it deserves to be and should get cleaned up. I'm mostly inclined to do that in a follow-up branch rather than block the bugfix behind it (either on this issue# or another). OTOH, I would like to avoid adding new bugs while fixing this one, and if we need cleanup to make the code human-readable enough for a review, I'm for it.

• find_objects_for_index scrubs @filters so it only includes exact matches on uuid and api_token.
• It also scrubs @where, but it allows any search on uuid. (This seems like a bug.)

Everything in @where is implicitly an equality operator, so "any search" is still just exact matches, right?

• If I'm reading [['uuid'], ['api_token']].include? filters right, current_api_client_is_trusted returns an error to the client if it tries to filter on any other attribute. It doesn't check the operator.

This seems especially confusing because what happens depends on the order these filters run in. I believe find_objects_for_index runs first, since it's declared first. If that's right, I believe the condition in current_api_client_is_trusted is a noop, because find_objects_for_index will have already scrubbed @filters by the time that check runs.

• close an information leak: without it, a query for "scopes=[...]" could result in either 403 or "only the token you're using", depending on whether any other tokens exist with that same set of scopes.
• cater to Workbench's current way of deciding whether to offer "Share" buttons, i.e., "does filtering on scopes return 403?". Until we teach Workbench a better way, we have to return 403 when the sharing token is used to do an index query that returns only itself.

...so I've changed the code to "filters must include at least one of (uuid, api_token)" and added comments to explain.

Also de-duplicated the current_api_client_is_trusted code so it just looks at @objects regardless of the current action.

Now at ffd4f23

Tom Clegg wrote:

Brett Smith wrote:

Reviewing afe4660. All the new code looks good. I'm running tests now, but everything's passed so far. At most, I have one question about it. I also have some questions about some existing code and how it relates to these change. If you think these are out of scope, that's totally fair, but I wanted to raise these in the hopes of avoiding another go-around on this ticket if we can help it.

I agree this area seems more complicated than it deserves to be and should get cleaned up. I'm mostly inclined to do that in a follow-up branch rather than block the bugfix behind it (either on this issue# or another). OTOH, I would like to avoid adding new bugs while fixing this one, and if we need cleanup to make the code human-readable enough for a review, I'm for it.

No, readability per se was never my concern. Addressing those concerns separately is fine.

• It also scrubs @where, but it allows any search on uuid. (This seems like a bug.)

Everything in @where is implicitly an equality operator, so "any search" is still just exact matches, right?

If I'm reading apply_where_limit_order_pararms correctly, you can use a where parameter to do a LIKE search by passing in a value ["contains", substring]. So, I don't think so.

Separately, shouldn't this limitation allow api_token through, the same way we consistently do with filters?

Everything else makes sense and looks good.

Brett Smith wrote:

If I'm reading apply_where_limit_order_pararms correctly, you can use a where parameter to do a LIKE search by passing in a value ["contains", substring]. So, I don't think so.

Separately, shouldn't this limitation allow api_token through, the same way we consistently do with filters?

Indeed. Somehow I missed that "contains" introduction. Well, I'd prefer to remove that feature (we can do that stuff with filters now without the ambiguous syntax) but this has already crept far enough so I've settled for making the "where" check equivalent to the "filters" check (including the api_token discrepancy) like this:

      @where.select! { |attr, val|
        val.is_a?(String) && (attr == 'uuid' || attr == 'api_token')
      }

→ 11fc089

A comment about the purpose of val.is_a?(String) might be helpful to future readers. Please go ahead and merge. Thanks.