Skip to content
Snippets Groups Projects

Filter out SAML enforced projects from GraphQL queries

All threads resolved!

What does this MR do and why?

Related to #514406

If you are a member of a group with group SAML enabled and sign in with regular password authentication you don't see any of the SAML projects. We do this by preventing the :read_project ability in ee/app/policies/ee/project_policy.rb#L786. The problem is when it comes to GraphQL queries they are filtered out after the SQL query which gives an incorrect count and messes up the pagination.

We first noticed this issue in #461083 (closed) and fixed it in the Organization related GraphQL queries in !160592 (merged).

We then gave the same treatment to the project GraphQL queries in !179650 (merged), !179651 (merged), and !179111 (merged).

What was missed is that we weren't doing the same checks as in ee/lib/gitlab/auth/group_saml/sso_enforcer.rb#L91 and therefore it caused a bug when using the GraphQL query with a PAT because we don't check SAML SSO status on APIs with a PAT. We reverted this bug in !180319 (merged)

This MR re-implements the GraphQL SAML filter using the logic in ee/lib/gitlab/auth/group_saml/sso_enforcer.rb#L91 and puts it behind a feature flag to de-risk the change.

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

When signed in with SAML SSO

No changes

Before After
Screenshot_2025-02-11_at_1.33.55_PM Screenshot_2025-02-11_at_1.07.33_PM

When signed in with password authentication

Before After
Screenshot_2025-02-11_at_1.34.42_PM Screenshot_2025-02-11_at_1.06.44_PM

How to set up and validate locally

  1. Setup SAML in GDK - https://gitlab.com/gitlab-org/gitlab-development-kit/blob/main/doc/howto/saml.md
  2. Start GDK in SaaS mode - https://docs.gitlab.com/ee/development/ee_features.html#simulate-a-saas-instance
  3. Create a group
  4. In the admin area give that group an Ultimate license
  5. Configure SAML for that group - https://gitlab.com/gitlab-org/gitlab-development-kit/blob/main/doc/howto/saml.md
  6. Create a few private projects in that group
  7. Sign in with SAML
  8. Create a few personal projects
  9. Go to /-/user_settings/password/edit and set a password
  10. Sign out, then sign back in with your password
  11. Go to /rails/features and enable the your_work_projects_vue feature flag
  12. Go to /dashboard/projects/member
  13. The count should only show the non-SAML projects

PAT authentication still works

  1. Go to /-/user_settings/personal_access_tokens and create a PAT
  2. In your terminal run the following (swapping in your PAT)
curl "https://gdk.test:3443/api/graphql" --header "Authorization: Bearer <your pat>" \
  --header "Content-Type: application/json" --request POST \
  --data "{\"query\": \"query {projects(membership: true) {nodes {nameWithNamespace}}}\"}"
  1. You should see SAML projects in the output.
Edited by Peter Hegman

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
  • 1 Message
    :book: CHANGELOG missing:

    If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog.

    If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

    Reviewer roulette

    Category Reviewer Maintainer
    backend @fdegier profile link current availability (UTC+1, 8 hours ahead of author) @schin1 profile link current availability (UTC+8, 15 hours ahead of author)
    database @subashis profile link current availability (UTC-7, same timezone as author) @bwill profile link current availability (UTC-6, 1 hour ahead of author)

    Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

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

    Generated by :no_entry_sign: Danger

    Edited by ****
  • Peter Hegman added 1 commit

    added 1 commit

    • 6dd8e2a1 - Filter out SAML enforced projects from GraphQL queries

    Compare with previous version

  • Peter Hegman mentioned in issue #514406

    mentioned in issue #514406

  • Peter Hegman changed the description

    changed the description

  • Peter Hegman requested review from @bhrai

    requested review from @bhrai

  • Peter Hegman requested review from @sgarg_gitlab

    requested review from @sgarg_gitlab

  • Smriti Garg
  • Bishwa Hang Rai approved this merge request

    approved this merge request

  • 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-cng: :white_check_mark: test report for 03ce3fc2

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Package     | 29     | 0      | 15      | 0     | 44    | ✅     |
    | Create      | 140    | 0      | 22      | 0     | 162   | ✅     |
    | Verify      | 52     | 0      | 20      | 0     | 72    | ✅     |
    | Ai-powered  | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Data Stores | 33     | 0      | 10      | 0     | 43    | ✅     |
    | Govern      | 84     | 0      | 10      | 0     | 94    | ✅     |
    | Plan        | 86     | 0      | 8       | 0     | 94    | ✅     |
    | Configure   | 0      | 0      | 3       | 0     | 3     | ➖     |
    | Secure      | 3      | 0      | 5       | 0     | 8     | ✅     |
    | Monitor     | 8      | 0      | 12      | 0     | 20    | ✅     |
    | Release     | 5      | 0      | 1       | 0     | 6     | ✅     |
    | Manage      | 1      | 0      | 9       | 0     | 10    | ✅     |
    | ModelOps    | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Fulfillment | 2      | 0      | 7       | 0     | 9     | ✅     |
    | Growth      | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 445    | 0      | 127     | 0     | 572   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-test-on-gdk: :white_check_mark: test report for 03ce3fc2

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Verify      | 102    | 0      | 42      | 0     | 144   | ✅     |
    | Create      | 270    | 0      | 46      | 0     | 316   | ✅     |
    | Package     | 48     | 0      | 28      | 0     | 76    | ✅     |
    | Plan        | 164    | 0      | 16      | 0     | 180   | ✅     |
    | Manage      | 2      | 0      | 18      | 0     | 20    | ✅     |
    | Data Stores | 66     | 0      | 20      | 0     | 86    | ✅     |
    | Govern      | 158    | 0      | 26      | 0     | 184   | ✅     |
    | Release     | 10     | 0      | 2       | 0     | 12    | ✅     |
    | Fulfillment | 4      | 0      | 14      | 0     | 18    | ✅     |
    | Monitor     | 16     | 0      | 24      | 0     | 40    | ✅     |
    | Secure      | 10     | 0      | 6       | 0     | 16    | ✅     |
    | Ai-powered  | 0      | 0      | 4       | 0     | 4     | ➖     |
    | Configure   | 0      | 0      | 6       | 0     | 6     | ➖     |
    | ModelOps    | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Growth      | 0      | 0      | 4       | 0     | 4     | ➖     |
    | Analytics   | 4      | 0      | 0       | 0     | 4     | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 854    | 0      | 258     | 0     | 1112  | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    Edited by ****
  • Peter Hegman added 674 commits

    added 674 commits

    Compare with previous version

  • Peter Hegman reset approvals from @bhrai by pushing to the branch

    reset approvals from @bhrai by pushing to the branch

  • Smriti Garg approved this merge request

    approved this merge request

  • Smriti Garg requested review from @schin1

    requested review from @schin1

  • Peter Hegman added 1 commit

    added 1 commit

    Compare with previous version

  • Peter Hegman reset approvals from @sgarg_gitlab by pushing to the branch

    reset approvals from @sgarg_gitlab by pushing to the branch

  • Peter Hegman requested review from @bhrai

    requested review from @bhrai

  • Sylvester Chin approved this merge request

    approved this merge request

  • Smriti Garg approved this merge request

    approved this merge request

  • 🤖 GitLab Bot 🤖 changed milestone to %17.10

    changed milestone to %17.10

  • Peter Hegman requested review from @OmarQunsulGitlab and removed review request for @bhrai

    requested review from @OmarQunsulGitlab and removed review request for @bhrai

  • Omar Qunsul approved this merge request

    approved this merge request

  • Omar Qunsul resolved all threads

    resolved all threads

  • Omar Qunsul enabled automatic add to merge train when checks pass

    enabled automatic add to merge train when checks pass

  • merged

  • Hello @peterhegman :wave:

    The database team is looking for ways to improve the database review process and we would love your help!

    If you'd be open to someone on the database team reaching out to you for a chat, or if you'd like to leave some feedback asynchronously, just post a reply to this comment mentioning:

    @gitlab-org/database-team

    And someone will be by shortly!

    Thanks for your help! :heart:

    This message was generated automatically. Improve it or delete it.

  • Omar Qunsul mentioned in commit a9086ebf

    mentioned in commit a9086ebf

  • added workflowstaging label and removed workflowcanary label

  • Please register or sign in to reply
    Loading