Make accessibility checks more robust for Reviews and Comments queries

Review Request #12424 — Created June 29, 2022 and submitted — Latest diff uploaded


Review Board


Recently, it was discovered that when querying for reviews there may be
duplicate Q(public=True)'s that appear in the queries to fetch the reviews.
The cause of this duplication lies in ReviewManager's _query method, which
handles the logic for querying for reviews. If an unauthenticated or anonymous
user was querying for the reviews, the public query field would be set twice,
once to the public value that was passed and once to True, with the latter
ensuring that only public reviews get returned to unauthenticated/anonymous
users. This however makes the query less efficient and can lead to the wrong
results being included in the returned queryset if a False value for public
was passed in.

To fix this, we ensure that the public query field is only set once, for any
type of user, in ReviewManager._query. This fix was applied to
CommentManager._query as well.

This change also adds a public parameter to ReviewManager.accessible to
ensure that the public parameter in ReviewManager._query handles all of
the logic surrounding public and unpublished reviews. Previously, you would
have to set the public query field using the extra_query parameter in
ReviewManager.accessible, which could also lead to duplicate or
conflicting Q(public= queries.

Finally, this change also improves the tests for the ReviewManager and
CommentManager. The tests are more clear, organized and well documented,
they use assertQueries, and have more comprehensive test coverage surrounding
access control.

  • Added new unit tests for ReviewManager.accessible with setting the
    public parameter.
  • Ran all unit tests with success.