Skip to content

Open issues count should not include hidden issues for non-admins

Serena Fang requested to merge fix-open-issues-count into master

What does this MR do?

Epic: &5741

Issue: #327355 (closed)

Follow up to: !66687 (merged)

Display the correct number of issues in the sidebar open issue count.

  • If the current user is an admin, the count includes all issues.
  • If the current user is a reporter or above (so they can see confidential issues), the count includes confidential issues but not hidden issues.
  • If the current user is a Guest or not a member, the count does not include confidential issues or hidden issues.

Cache the issue count so it doesn't have to be constantly recalculated. The count is stored in 3 cache keys:

  • TOTAL_COUNT_KEY includes confidential and hidden issues (admin)
  • TOTAL_COUNT_WITHOUT_HIDDEN_KEY includes confidential issues but not hidden issues (reporter and above)
  • PUBLIC_COUNT_WITHOUT_HIDDEN_KEY does not include confidential or hidden issues (guest)

Background

Admins can ban suspicious users. Banning a user blocks them and hides their issues from non-admins until the issues can be reviewed and either deemed safe or deleted.

The sidebar open issues count showed the wrong number of issues for non-admins, since hidden issues would be included in the sidebar count but not appear in the issues list. In the screenshot below, this project has a public issue and a hidden issue. When signed in as a non-member, the issue count includes both issues, but should only include the public issue.

Screen_Shot_2021-08-26_at_2.16.33_PM

Feature implementation plan

The plan: !64728 (comment 623492866)

  1. Create banned_users table. ✔ !64728 (merged)
  2. Update the app so that when user is banned new record is inserted in banned_users (and the opposite - when user is un-banned, the record is removed). ✔ !64728 (merged)
  3. Update issues finder to exclude results from banned users. Put this change behind feature flag. ✔ !66687 (merged)
  4. Enable the feature flag. #330667 (closed) ✔
  5. Monitor performance (e.g. visualization in Grafana).
  6. Continue work towards updating issues.hidden with background worker, and filter using this column. It's important not to ignore this step, so that we are not caught by surprise when banned_users gets to a size that will affect performance.

DB:

First create some banned users so there are hidden issues:

exec INSERT INTO banned_users SELECT NOW(), NOW(), id FROM users WHERE id > (SELECT id FROM USERS ORDER BY id DESC OFFSET 1000 LIMIT 1) AND (floor(random() * 100)::int > 80)

Admin:

Query: Issue.opened.with_issue_type(Issue::TYPES_FOR_LIST).where(project: projects)

SQL: explain SELECT "issues".* FROM "issues" WHERE "issues"."project_id" = 278964 AND ("issues"."state_id" IN (1)) AND "issues"."issue_type" IN (0, 1) ORDER BY "issues"."created_at" DESC, "issues"."id" DESC LIMIT 20 OFFSET 0

Plan: https://explain.dalibo.com/plan/sAT

Reporter+:

Query: Issue.without_hidden.opened.with_issue_type(Issue::TYPES_FOR_LIST).where(project: projects)

SQL: EXPLAIN SELECT “issues”.* FROM “issues” WHERE (NOT EXISTS (SELECT 1 FROM “banned_users” WHERE (issues.author_id = banned_users.user_id))) AND “issues”.“project_id” = 20 AND (“issues”.“state_id” IN (1)) AND “issues”.“issue_type” IN (0, 1) ORDER BY “issues”.“created_at” DESC, “issues”.“id” DESC LIMIT 20 OFFSET 0

Plan: https://explain.dalibo.com/plan/bw8

Guest/non-member:

Query: Issue.without_hidden.public_only.opened.with_issue_type(Issue::TYPES_FOR_LIST).where(project: projects)

SQL: EXPLAIN SELECT “issues”.* FROM “issues” WHERE (NOT EXISTS (SELECT 1 FROM “banned_users” WHERE (issues.author_id = banned_users.user_id))) AND ( issues.confidential IS NOT TRUE OR (issues.confidential = TRUE AND (issues.author_id = 5966677 OR EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = 5966677 AND issue_id = issues.id) OR EXISTS (SELECT 1 FROM “project_authorizations” WHERE “project_authorizations”.“user_id” = 5966677 AND (project_authorizations.project_id = issues.project_id) AND (project_authorizations.access_level >= 20))))) AND “issues”.“project_id” = 278964 AND (“issues”.“state_id” IN (1)) AND “issues”.“issue_type” IN (0, 1) ORDER BY “issues”.“created_at” DESC, “issues”.“id” DESC LIMIT 20 OFFSET 0

Plan: https://explain.dalibo.com/plan/9NK

Screenshots or Screencasts (strongly suggested)

When admin:

Counts all issues including public, confidential, and hidden issues (4)

image

When reporter:

Counts public and confidential issues, but not hidden issues (2)

image

When non-member:

Counts public issues, but not hidden or confidential issues (1)

image

How to setup and validate locally (strongly suggested)

  1. Enable the banned users feature in the Rails console:
Feature.enable(:ban_user_feature_flag)
  1. Create a public project.
  2. Create a public issue and confidential issue in the project.
  3. Impersonate a user and create a public issue and confidential issue in the project.
  4. Stop impersonation.
  5. Ban the user (user's page > Settings > Ban user)
  6. View the project page as an admin, a reporter, and a guest. For the admin, the sidebar issues count should be 4 (public, confidential, hidden, hidden+confidential). For the reporter, the sidebar issues count should be 2 (public, confidential). For the guest, the sidebar issues count should be 1 (public).

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Mayra Cabrera

Merge request reports