-
Notifications
You must be signed in to change notification settings - Fork 2
Add GraphQL customer authorization utilities (issue #761) #767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #767 +/- ##
==========================================
+ Coverage 75.91% 76.13% +0.21%
==========================================
Files 72 73 +1
Lines 19935 20324 +389
==========================================
+ Hits 15134 15473 +339
- Misses 4801 4851 +50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
df808ee to
cdc7a19
Compare
sophie-cluml
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see that PR contains common building block functions for upcoming tasks and I think it is highly beneficial to have robust testing that covers not just success paths, but also dangerous, edge, and boundary cases. I suggest adding the following two specific test cases, and I would like to ask you to also check if there are any other cases that should be covered.
process_load_edges_filtered: Verify thathas_next/has_previousflags work correctly when all data is filtered out by the predicate, resulting in an empty list.users_customers: It seems like we need test code for this.
cdc7a19 to
1093774
Compare
|
Applied the suggested adjustment. |
src/graphql/customer_access.rs
Outdated
| let user = account_map | ||
| .get(username)? | ||
| .ok_or_else::<async_graphql::Error, _>(|| "User not found".into())?; | ||
| Ok(user.customer_ids.clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Ok(user.customer_ids.clone()) | |
| Ok(user.customer_ids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated accordingly.
1093774 to
4410458
Compare
|
@sehkone — Friendly ping: PR #767 (Add GraphQL customer authorization utilities) is approved (final approval 2026-01-18), all CI/checks are passing, and the PR is not a draft. It has been ready for merge for more than 7 days but shows mergeable_state: blocked (branch protection rules preventing merge). Could you please take a look and either merge it or advise what action is needed (e.g., adjust branch-protection requirements, grant merge rights, or request a specific additional review)? If you prefer, I can rebase or make any tiny adjustments required to satisfy branch-protection rules — please let me know the preferred next step. Thanks! |
|
@danbi2990 Could you rebase upon main please? |
2 files changed, 5 insertions(+) Automated changes made by octoaide bot.
4410458 to
a196615
Compare
|
Rebase completed. |
This PR implements a small helper module to centralize customer-scoping and authorization logic used by the GraphQL layer (graphql::auth), addressing issue #761.
Closes #761
Summary of changes:
Add new module src/graphql/auth.rs providing customer-scoping utilities for GraphQL handlers.
Export auth module from src/graphql.rs so it is available as crate::graphql::auth.
Add unit tests covering admin behavior, membership checks, intersection logic, iterator filtering, and unauthorized error.
Update CHANGELOG.md with an entry describing the new utilities.
Why this change:
Files changed (high level):
Notes:
If you want any naming or API adjustments (e.g., using a different integer type for customer IDs), I can update the implementation accordingly.