Filter out SAML enforced projects from GraphQL queries
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 |
---|---|
![]() |
![]() |
When signed in with password authentication
Before | After |
---|---|
![]() |
![]() |
How to set up and validate locally
- Setup SAML in GDK - https://gitlab.com/gitlab-org/gitlab-development-kit/blob/main/doc/howto/saml.md
- Start GDK in SaaS mode - https://docs.gitlab.com/ee/development/ee_features.html#simulate-a-saas-instance
- Create a group
- In the admin area give that group an Ultimate license
- Configure SAML for that group - https://gitlab.com/gitlab-org/gitlab-development-kit/blob/main/doc/howto/saml.md
- Create a few private projects in that group
- Sign in with SAML
- Create a few personal projects
- Go to
/-/user_settings/password/edit
and set a password - Sign out, then sign back in with your password
- Go to
/rails/features
and enable theyour_work_projects_vue
feature flag - Go to
/dashboard/projects/member
- The count should only show the non-SAML projects
PAT authentication still works
- Go to
/-/user_settings/personal_access_tokens
and create a PAT - 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}}}\"}"
- You should see SAML projects in the output.
Merge request reports
Activity
changed milestone to %17.9
assigned to @peterhegman
added pipelinetier-1 label
added database databasereview pending feature flag labels
- Resolved by Peter Hegman
1 Message 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
(UTC+1, 8 hours ahead of author)
@schin1
(UTC+8, 15 hours ahead of author)
database @subashis
(UTC-7, same timezone as author)
@bwill
(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
danger-review
job that generated this comment.Generated by
DangerEdited by ****added 1 commit
- 6dd8e2a1 - Filter out SAML enforced projects from GraphQL queries
mentioned in issue #514406
- Resolved by Sylvester Chin
@bhrai this triggered a database review but the queries are not changed in this MR. They were changed in !179650 (merged), !179651 (merged), and !179111 (merged) and all of those got database review. When we revered we didn't remove the filter in the finder we disabled it for the GraphQL queries that were causing issues. Anyway I haven't added raw SQL and query plans because I was thinking we could use the ones in the MRs I linked above. Would you be able to review this and is that okay with you?
requested review from @bhrai
- Resolved by Omar Qunsul
@sgarg_gitlab can you help with initial backend review of this since it is related to groupauthentication?
Let me know if you need additional context about anything!
requested review from @sgarg_gitlab
- Resolved by Sylvester Chin
added databaseapproved label and removed databasereview pending label
added pipeline:mr-approved label
added pipelinetier-3 pipeline:run-e2e-omnibus-once labels 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-cng:
test report for 03ce3fc2expand 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:
test report for 03ce3fc2expand 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 ****added 674 commits
-
6dd8e2a1...0c9aa9f9 - 673 commits from branch
master
- a4d5680b - Filter out SAML enforced projects from GraphQL queries
-
6dd8e2a1...0c9aa9f9 - 673 commits from branch
reset approvals from @bhrai by pushing to the branch
requested review from @schin1
- Resolved by Sylvester Chin
reset approvals from @sgarg_gitlab by pushing to the branch
requested review from @bhrai
changed milestone to %17.10
added missed:17.9 label
requested review from @OmarQunsulGitlab and removed review request for @bhrai
started a merge train
Hello @peterhegman
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!
This message was generated automatically. Improve it or delete it.
mentioned in commit a9086ebf
added workflowstaging-canary label and removed workflowin dev label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-production label and removed workflowproduction label