670722 - Unreviewed add-ons should not appear in author page, "Other add-ons by author" or "Often used with"

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect, P2)

Description

[Jorge Villalobos [:jorgev] (he/him)]

Unreviewed add-ons should only be accessible through direct linking. Apparently, we're displaying them on different places in the site:
* The developer's user page.
* The "Other add-ons by this author" section in an add-on listing.
* Possibly the "Often used with" section in an add-on listing.

It is important for security that they don't show up anywhere. Unreviewed add-ons by popular authors could get lots of attention because of this.

Comment 1

[Wil Clouser [:clouserw]]

Pretty sure this is a dupe of a bug that chenb already has a patch for

Comment 2

[Wil Clouser [:clouserw]]

Not quite a dupe of bug 504685 but close.  -> chenba for this one too.  Let me know if you won't have time for this.

Comment 3

[Barry Chen]

Commit @ https://github.com/chenba/zamboni/commit/579a73edd2f98f02d716156168b6b4e600288042 and pull request @ https://github.com/jbalogh/zamboni/pull/196

Comment 4

[Barry Chen]

New commit @ https://github.com/chenba/zamboni/commit/82438b82ce4e21e8b551060760f89434395d2857 and pull request is still @ https://github.com/jbalogh/zamboni/pull/196

As for the two unit tests that were failing because of the patch for this bug, the count that was tested against (10) was incorrect.  There were 13 addons included by the tests from fixtures.  10 with STATUS_PUBLIC and 3 with STATUS_UNREVIEWED.  The query (post-patch) in the tests was selecting for addons with a listed status or disabled status with a current version.  Nine addons matched the query conditions.  It was previously 10 because the unreviewed status was also being selected and there's one addon with a unreviewed status _and_ a current version.

Moreover, it appears that AddonManager.valid_and_disabled() is incorrect or TestAddonManager.test_valid_disabled_by_user() is testing the wrong method.  As mentioned above, the query produced by valid_and_disabled() selects for listed or disabled statuses and null current_version; it does not check the disabled by user (`inactive`) field.  The test passes because it uses a hardcoded number.  It will fail if it checks to see if the number of valid and disabled addons decreased by one after one of the addons is updated with disabled_by_user=True.  I've submitted this as bug 674839.

Comment 5

[Jeff Balogh (:jbalogh)]

https://github.com/jbalogh/zamboni/commit/852b38e 

Thanks Barry!

Comment 6

[Jeff Balogh (:jbalogh)]

Reverted because tests broke. https://jenkins.mozilla.org/job/amo-master/3736/

Comment 7

[Barry Chen]

I looked into the three failed tests Andy told me about (https://github.com/jbalogh/zamboni/pull/196#issuecomment-1671792): TestStatus.test_unreviewed, TestStatus.test_purgatory, and TestStatus.test_nominated in the addons view tests.

They fail because with the patch, STATUS_UNREVIEWED, STATUS_PURGATORY, and STATUS_NOMINATED are no longer "listed" statuses.  And addon_detail() view method uses the @addon_disabled_view decorator, which in turn uses the valid_and_disabled() filter.  Since they are not listed and filtered out by valid_and_disabled(), the tests are seeing 404s.

I understand that unreviewed addons need to be accessible through direct linking, so the tests are correct.  If "listed" is not the same as publicly accessible, then we need to update valid_and_disabled() to use a different set of statuses, or use a different filter.

Please advise.

Comment 8

[Barry Chen]

The other three failing test linked by jbalogh in comment 6 are caused by the same reason I mentioned in comment 7.  Addons 228106 and 228107 from the fixtures both have a STATUS_UNREVIEWED, and test_unreviewed_robots() updates addon 3615's status to STATUS_UNREVIEWED during the test, causing a couple of 404s after that.

Comment 9

[Jeff Balogh (:jbalogh)]

Unreviewed addons should not show up in search and other listing pages, but they should be accessible through direct links to the detail page. The failing tests are catching regressions.

Comment 10

[Barry Chen]

(In reply to comment #9)
> Unreviewed addons should not show up in search and other listing pages, but
> they should be accessible through direct links to the detail page. The
> failing tests are catching regressions.

I understand that.  The issues is that currently the "listed" statuses are also treated as accessible statuses.  When the patch fixes the listed statuses by excluding the unreviewed statuses, the unreviewed addons are no longer accessible.

Comment 11

[Jeff Balogh (:jbalogh)]

I'd just make a .reviewed() on the manager that only looks at REVIEWED_STATUSES.

Comment 12

[Barry Chen]

New patch @ https://github.com/chenba/zamboni/commit/eef6798fa92aa2c70b0dee1dac98b8587b8f0ad0 and pull request remains @ https://github.com/jbalogh/zamboni/pull/196.

There are three set of changes in this patch.

1) Added the .reviewed() as suggested by jbalogh in comment 11.  User.addons_listed() now uses that.  This effectively fixes this bug.

2) But since LISTED_STATUSES was incorrect, that's been fixed.

3) ACCESSIBLE_STATUSES was created because LISTED_STATUSES no longer includes a few statuses that should be accessible.  The ACCESSIBLE_STATUSES are based on the TestStatus tests in addons view tests.

I also updated valid_and_disabled() in AddonManager to include addons disabled by users.

Comment 13

[Wil Clouser [:clouserw]]

Thanks chenba.  Jeff, can you review again? thanks

Comment 14

[Barry Chen]

What's the status on this?  If the scope of the patch is too big, I can limit it to just the .reviewed() changes (see comment 12).  Thanks!

Comment 15

[Jeff Balogh (:jbalogh)]

I just took the .reviewed() part. https://github.com/jbalogh/zamboni/commit/969ba69 

Thanks!