Skip to content
Snippets Groups Projects

Redirect group searches when SSO restricted

Merged Terri Chu requested to merge 398572-reauth-for-group-and-project-search into master
All threads resolved!

What does this MR do and why?

Notes for reviewer:

  • I originally implemented this for group and project searches, but in testing found that only group searches have this issue. Project search does not appear to be restricted by Group SSO.
  • Global searches cannot be handled in this manner and will be handled separately

This MR introduces a redirect for group search when the SSO login has expired for the group. This is behind a derisk feature flag

References

Please include cross links to any resources that are relevant to this MR. This will give reviewers and future readers helpful context to give an efficient review of the changes introduced.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Group search results when logged in

image

Before After
image image

How to set up and validate locally

gdk requires a good bit of setup to get this working, I don't think Advanced search is required, but I have it setup to test this

  1. enable ff: search_group_sso_redirect
  2. setup elasticsearch (advanced search) in gdk
  3. Setup HTTPS in gdk: https://gitlab.com/gitlab-org/gitlab-development-kit/-/blob/main/doc/howto/nginx.md
  4. Setup SAML in gdk: https://gitlab.com/gitlab-org/gitlab-development-kit/-/blob/main/doc/howto/saml.md#saml
    • note DO NOT configure the group yet
  5. Simulate SaaS in gdk: https://docs.gitlab.com/ee/development/ee_features.html#simulate-a-saas-instance
    • I added an env.runit file in the root gdk directory (not gitlab)
    • restart gitlab
  6. setup a private group, make sure code is there and is searchable
  7. add a non-admin user to the group as a developer+ role
  8. login with that user
  9. search at the group level, make sure you get results
  10. follow guide to add SAML to the group: https://gitlab.com/gitlab-org/gitlab-development-kit/-/blob/main/doc/howto/saml.md#configuring-the-group
  11. re-run the search, make sure you are redirected to SSO login screen (this is the fix)
Edited by Terri Chu

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
  • SAM FIGUEROA requested changes

    requested changes

  • Terri Chu mentioned in merge request !179114 (merged)

    mentioned in merge request !179114 (merged)

  • Terri Chu added 585 commits

    added 585 commits

    • 53707251...b4193c77 - 582 commits from branch master
    • 24bab027 - Redirect group searches when SSO enabled and expired
    • f3f82608 - Adjust check and add feature flag
    • 873c1897 - Use SsoEnforcer::access_restricted? and update specs

    Compare with previous version

  • Terri Chu requested review from @sam.figueroa

    requested review from @sam.figueroa

  • Terri Chu
  • SAM FIGUEROA approved this merge request

    approved this merge request

  • SAM FIGUEROA requested review from @dgruzd

    requested review from @dgruzd

  • added pipelinetier-2 label and removed pipelinetier-1 label

  • Before you set this MR to auto-merge

    This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3. We will trigger a new pipeline for each transition to a higher tier.

    Before you set this MR to auto-merge, please check the following:

    • You are the last maintainer of this merge request
    • The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
    • This pipeline is recent enough (created in the last 8 hours)

    If all the criteria above apply, please set auto-merge for this merge request.

    See pipeline tiers and merging a merge request for more details.

  • E2E Test Result Summary

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :white_check_mark: test report for 873c1897

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Verify      | 104    | 0      | 40      | 2     | 144   | ✅     |
    | Govern      | 158    | 0      | 26      | 0     | 184   | ✅     |
    | Create      | 276    | 0      | 40      | 0     | 316   | ✅     |
    | Plan        | 164    | 0      | 16      | 0     | 180   | ✅     |
    | Package     | 48     | 0      | 28      | 0     | 76    | ✅     |
    | Data Stores | 66     | 0      | 20      | 0     | 86    | ✅     |
    | Analytics   | 4      | 0      | 0       | 0     | 4     | ✅     |
    | Monitor     | 16     | 0      | 24      | 0     | 40    | ✅     |
    | Fulfillment | 4      | 0      | 14      | 0     | 18    | ✅     |
    | Secure      | 8      | 0      | 6       | 0     | 14    | ✅     |
    | Ai-powered  | 0      | 0      | 4       | 0     | 4     | ➖     |
    | Manage      | 2      | 0      | 18      | 0     | 20    | ✅     |
    | Configure   | 0      | 0      | 6       | 0     | 6     | ➖     |
    | ModelOps    | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Release     | 10     | 0      | 2       | 0     | 12    | ✅     |
    | Growth      | 0      | 0      | 4       | 0     | 4     | ➖     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 860    | 0      | 250     | 2     | 1110  | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-test-on-cng: :white_check_mark: test report for 873c1897

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Data Stores | 33     | 0      | 10      | 0     | 43    | ✅     |
    | Create      | 143    | 0      | 19      | 0     | 162   | ✅     |
    | Verify      | 53     | 0      | 19      | 0     | 72    | ✅     |
    | Plan        | 86     | 0      | 8       | 0     | 94    | ✅     |
    | Govern      | 84     | 0      | 10      | 0     | 94    | ✅     |
    | Fulfillment | 2      | 0      | 7       | 0     | 9     | ✅     |
    | Release     | 5      | 0      | 1       | 0     | 6     | ✅     |
    | Package     | 29     | 0      | 15      | 0     | 44    | ✅     |
    | Manage      | 1      | 0      | 9       | 0     | 10    | ✅     |
    | Secure      | 2      | 0      | 5       | 0     | 7     | ✅     |
    | Ai-powered  | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Monitor     | 8      | 0      | 12      | 0     | 20    | ✅     |
    | Configure   | 0      | 0      | 3       | 0     | 3     | ➖     |
    | Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Growth      | 0      | 0      | 2       | 0     | 2     | ➖     |
    | ModelOps    | 0      | 0      | 1       | 0     | 1     | ➖     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 448    | 0      | 123     | 0     | 571   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    Edited by ****
  • Dmitry Gruzd approved this merge request

    approved this merge request

  • Dmitry Gruzd resolved all threads

    resolved all threads

  • Dmitry Gruzd enabled automatic add to merge train when checks pass

    enabled automatic add to merge train when checks pass

  • merged

  • Dmitry Gruzd mentioned in commit 40179c4a

    mentioned in commit 40179c4a

  • added workflowstaging label and removed workflowcanary label

  • Terri Chu mentioned in merge request !180750 (merged)

    mentioned in merge request !180750 (merged)

  • Please register or sign in to reply
    Loading