Skip to content
Snippets Groups Projects

Fix group authorization when searching for epics

Merged Felipe Cardozo requested to merge issue_371067 into master
All threads resolved!

What does this MR do and why?

Follow up of !93601 (merged).

On the MR above I forgot to take into account projects that are visible to the user to get authorized groups, in other words, if a group has a project all group members can read ancestor groups.

More details at #371067 (closed)

This changes one of the queries of the union introduced by !93601 (merged), it gets a bit worse performance-wise, but we need to keep the same behavior from before at least until we remove this authorization inconsistency.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Felipe Cardozo

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Felipe Cardozo requested review from @dstull and @nicolasdular

    requested review from @dstull and @nicolasdular

  • Felipe Cardozo added 1 commit

    added 1 commit

    • f914051b - Fix group authorization when searching for epics

    Compare with previous version

  • Felipe Cardozo added 1 commit

    added 1 commit

    • 7614aea2 - Fix group authorization when searching epics

    Compare with previous version

  • Allure report

    allure-report-publisher generated test report!

    review-qa-blocking: :exclamation: test report for 7614aea2

    expand test summary
    +-----------------------------------------------------------------------------------------+
    |                                     suites summary                                      |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    |                                    | passed | failed | skipped | flaky | total | result |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    | Create                             | 28     | 0      | 1       | 11    | 29    | ❗     |
    | Protect                            | 2      | 0      | 0       | 2     | 2     | ❗     |
    | Plan                               | 47     | 0      | 1       | 27    | 48    | ❗     |
    | Verify                             | 12     | 0      | 1       | 10    | 13    | ❗     |
    | Manage                             | 46     | 0      | 3       | 24    | 49    | ❗     |
    | Configure                          | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Feature flag handler sanity checks | 9      | 0      | 0       | 0     | 9     | ✅     |
    | Package                            | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Version sanity check               | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Secure                             | 2      | 0      | 0       | 2     | 2     | ❗     |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    | Total                              | 146    | 0      | 9       | 76    | 155   | ❗     |
    +------------------------------------+--------+--------+---------+-------+-------+--------+

    e2e-review-qa-blocking: :exclamation: test report for 7614aea2

    expand test summary
    +-----------------------------------------------------------------------------------------+
    |                                     suites summary                                      |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    |                                    | passed | failed | skipped | flaky | total | result |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    | Package                            | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Plan                               | 47     | 0      | 1       | 31    | 48    | ❗     |
    | Create                             | 28     | 0      | 1       | 20    | 29    | ❗     |
    | Manage                             | 46     | 0      | 3       | 29    | 49    | ❗     |
    | Verify                             | 12     | 0      | 1       | 10    | 13    | ❗     |
    | Secure                             | 2      | 0      | 0       | 2     | 2     | ❗     |
    | Version sanity check               | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Feature flag handler sanity checks | 9      | 0      | 0       | 0     | 9     | ✅     |
    | Protect                            | 2      | 0      | 0       | 2     | 2     | ❗     |
    | Configure                          | 0      | 0      | 1       | 0     | 1     | ➖     |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    | Total                              | 146    | 0      | 9       | 94    | 155   | ❗     |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
  • Doug Stull requested review from @jdrpereira and removed review request for @dstull

    requested review from @jdrpereira and removed review request for @dstull

  • Nicolas Dular approved this merge request

    approved this merge request

  • Nicolas Dular requested review from @dgruzd and removed review request for @nicolasdular

    requested review from @dgruzd and removed review request for @nicolasdular

  • :wave: @nicolasdular, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.

    For more info, please refer to the following links:

  • Dmitry Gruzd approved this merge request

    approved this merge request

  • Dmitry Gruzd removed review request for @dgruzd

    removed review request for @dgruzd

  • João Pereira approved this merge request

    approved this merge request

  • Felipe Cardozo requested review from @lulalala

    requested review from @lulalala

  • Felipe Cardozo requested review from @sabrams

    requested review from @sabrams

  • Steve Abrams approved this merge request

    approved this merge request

  • added databaseapproved label and removed databasereviewed label

  • Steve Abrams resolved all threads

    resolved all threads

  • Steve Abrams enabled an automatic merge when the pipeline for f9f29eed succeeds

    enabled an automatic merge when the pipeline for f9f29eed succeeds

  • Reviewer roulette

    Changes that require review have been detected!

    Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

    Category Reviewer Maintainer
    backend Roy Zwambag (@rzwambag) (UTC+2, 5 hours ahead of @felipe_artur) Bob Van Landuyt (@reprazent) (UTC+2, 5 hours ahead of @felipe_artur)
    database João Pereira (@jdrpereira) (UTC+1, 4 hours ahead of @felipe_artur) Mayra Cabrera (@mayra-cabrera) (UTC-5, 2 hours behind @felipe_artur)

    To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

    To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

    Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

    If needed, you can retry the :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • merged

  • Steve Abrams mentioned in commit 54678818

    mentioned in commit 54678818

  • added workflowstaging label and removed workflowcanary label

  • Please register or sign in to reply
    Loading