fix(scopes): add new specific scopes for sponsor extra questions endp…#524
fix(scopes): add new specific scopes for sponsor extra questions endp…#524
Conversation
…oints chore(utils): add new helper trait to perform endpoint migrations
📝 WalkthroughWalkthroughAdds OAuth2 scopes, Swagger scope entries, controller metadata updates, DB migration and seeding for sponsor "extra-questions" endpoints, plus helper trait for idempotent API endpoint/scope migration SQL and a .gitignore update. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API (OAuth2SummitSponsorApiController)
participant Auth as OAuth2 Server
participant DB as Database (api_scopes / api_endpoints / authz)
Client->>API: Request sponsor extra-questions (GET/POST/PUT/DELETE)
API->>Auth: Validate access token + required scope(s)
Auth-->>API: Token valid / scope allowed
API->>DB: Query endpoint config (scopes, authz groups) / persist changes (migration/seeded)
DB-->>API: Config / result
API-->>Client: Return data / 403 if unauthorized
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-524/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Pull request overview
This PR introduces dedicated OAuth scopes for Summit Sponsor “extra questions” endpoints and wires them into the API endpoint metadata, Swagger security schema, and a config-DB migration so existing environments can receive the new scope/endpoint bindings without re-running seeders.
Changes:
- Add
ReadSponsorExtraQuestions/WriteSponsorExtraQuestionstoSummitScopesand seed them viaApiScopesSeeder. - Bind the new scopes (and add
SponsorExternalUsersgroup access) to sponsor extra-questions endpoints inApiEndpointsSeeder. - Add a config migration + reusable SQL helper trait to insert the new scopes and endpoint associations idempotently.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| routes/api_v1.php | Clarifies route grouping comment for sponsor extra questions. |
| database/seeders/ApiScopesSeeder.php | Seeds the two new sponsor extra-questions scopes. |
| database/seeders/ApiEndpointsSeeder.php | Adds new scopes (and group) to sponsor extra-questions endpoints. |
| database/migrations/config/Version20260408143000.php | Adds idempotent migration to apply the new scopes/bindings to existing envs. |
| database/migrations/config/APIEndpointsMigrationHelper.php | Adds reusable SQL templates for config migrations (endpoints/scopes/group bindings). |
| app/Swagger/Security/SponsorOAuth2Schema.php | Exposes new scopes in sponsor OAuth2 Swagger schema. |
| app/Security/SummitScopes.php | Defines new scope constants. |
| app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php | Updates OpenAPI annotations to include the new scope/group for sponsor extra-questions endpoints. |
| .gitignore | Ignores docs/ directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param string $apiName API identifier (e.g., 'summits') | ||
| * @param string $endpointName Endpoint identifier (e.g., 'get-sponsor-extra-questions') | ||
| * @param string $route Route pattern (e.g., '/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions') | ||
| * @param string $httpMethod HTTP method as serialized PHP array (e.g., 'a:1:{i:0;s:3:"GET";}') | ||
| * @param bool $active Whether the endpoint is active (default: true) | ||
| * @param bool $allowCors Whether to allow CORS (default: false) | ||
| * @param bool $allowCredentials Whether to allow credentials (default: false) | ||
| * @return string SQL INSERT statement | ||
| */ | ||
| protected function insertEndpoint( | ||
| string $apiName, | ||
| string $endpointName, | ||
| string $route, | ||
| string $httpMethod, | ||
| bool $active = true, | ||
| bool $allowCors = false, | ||
| bool $allowCredentials = false |
There was a problem hiding this comment.
insertEndpoint() docstring says http_method should be a serialized PHP array and defaults allowCors/allowCredentials to false. In this codebase endpoints are seeded with a plain method string (e.g. 'GET') and CORS/credentials enabled by default (see ApiEndpointsSeeder::seedApiEndpoints). If this helper is used as-written it will insert values that don't match existing conventions/behavior and may prevent endpoint lookup/authorization from working as expected. Consider updating the doc/parameter expectation to a plain HTTP method string and defaulting allow_cors/allow_credentials to true to match seeding behavior.
| * @param array $scopes List of scope URIs to remove associations for | ||
| * @return string SQL DELETE statement | ||
| */ | ||
| protected function deleteScopesEndpoints(array $scopes): string | ||
| { | ||
| $scopeList = "'" . implode("', '", $scopes) . "'"; | ||
| return <<<SQL | ||
| DELETE eas FROM endpoint_api_scopes eas | ||
| INNER JOIN api_scopes s ON s.id = eas.scope_id | ||
| WHERE s.name IN ({$scopeList}); |
There was a problem hiding this comment.
deleteScopesEndpoints() deletes endpoint-scope associations by scope name without constraining by API. Since api_scopes.name is not globally unique, this can remove associations for other APIs that happen to reuse the same scope URI. Consider either (a) adding an $apiName parameter and joining through apis/api_scopes.api_id, or (b) deleting via scope IDs resolved for the intended API only.
| * @param array $scopes List of scope URIs to remove associations for | |
| * @return string SQL DELETE statement | |
| */ | |
| protected function deleteScopesEndpoints(array $scopes): string | |
| { | |
| $scopeList = "'" . implode("', '", $scopes) . "'"; | |
| return <<<SQL | |
| DELETE eas FROM endpoint_api_scopes eas | |
| INNER JOIN api_scopes s ON s.id = eas.scope_id | |
| WHERE s.name IN ({$scopeList}); | |
| * @param string $apiName API identifier | |
| * @param array $scopes List of scope URIs to remove associations for | |
| * @return string SQL DELETE statement | |
| */ | |
| protected function deleteScopesEndpoints(string $apiName, array $scopes): string | |
| { | |
| $scopeList = "'" . implode("', '", $scopes) . "'"; | |
| return <<<SQL | |
| DELETE eas FROM endpoint_api_scopes eas | |
| INNER JOIN api_scopes s ON s.id = eas.scope_id | |
| INNER JOIN apis a ON a.id = s.api_id | |
| WHERE a.name = '{$apiName}' | |
| AND s.name IN ({$scopeList}); |
| 'http_method' => 'POST', | ||
| 'scopes' => [ | ||
| SummitScopes::WriteSummitData, | ||
| SummitScopes::WriteSponsorExtraQuestions, | ||
| ], |
There was a problem hiding this comment.
This endpoint now requires SummitScopes::WriteSponsorExtraQuestions, but the corresponding OpenAPI annotation for addSponsorExtraQuestion in OAuth2SummitSponsorApiController still lists only WriteSummitData in its security scopes (around the #[OA\Post(... operationId: 'addSponsorExtraQuestion')] block). This mismatch will cause generated docs/clients to omit the required scope; please update the controller annotation to include the new scope.
| IGroup::SuperAdmins, | ||
| IGroup::Administrators, | ||
| IGroup::SummitAdministrators, | ||
| IGroup::Sponsors, | ||
| IGroup::SponsorExternalUsers, |
There was a problem hiding this comment.
The #[OA\Get(...)] description string still lists required groups only up to IGroup::Sponsors, but the x.required-groups array now includes IGroup::SponsorExternalUsers. Consider updating the description string to include the external users group too, to keep the annotation consistent.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php (1)
3423-3429:⚠️ Potential issue | 🟠 MajorMissing
WriteSponsorExtraQuestionsscope inaddExtraQuestionendpoint.The migration at
Version20260408143000.phpassociatesadd-sponsor-extra-questionwith the new write scope, but this controller annotation is missingSummitScopes::WriteSponsorExtraQuestions. All other write endpoints (updateExtraQuestion,deleteExtraQuestion,addExtraQuestionValue, etc.) include the new scope.🐛 Proposed fix
security: [ [ 'summit_sponsor_oauth2' => [ SummitScopes::WriteSummitData, + SummitScopes::WriteSponsorExtraQuestions, ] ] ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php` around lines 3423 - 3429, The security annotation for the addExtraQuestion endpoint in OAuth2SummitSponsorApiController.php is missing the new WriteSponsorExtraQuestions scope; update the security array used by the addExtraQuestion method to include SummitScopes::WriteSponsorExtraQuestions alongside SummitScopes::WriteSummitData so the endpoint matches the migration and other write endpoints (e.g., updateExtraQuestion, deleteExtraQuestion, addExtraQuestionValue).
🧹 Nitpick comments (2)
.gitignore (1)
42-42: Narrowdocs/ignore to generated artifacts only.At Line 42,
docs/ignores the entire documentation tree, which can hide legitimate doc/source updates in future PRs. If the intent is to ignore generated output, prefer a specific path (for exampledocs/build/) instead of the whole directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 42, The .gitignore currently ignores the entire docs/ tree which hides source doc changes; update the ignore entry to target only generated artifacts (e.g., replace or remove the generic "docs/" entry and add a specific path like "docs/build/" or "docs/_site/" as appropriate) so documentation source files remain tracked; ensure you update the .gitignore entry that currently contains "docs/" to the specific generated-directory name used by your build tooling.database/migrations/config/APIEndpointsMigrationHelper.php (1)
48-68: SQL string interpolation without escaping in migration helpers.While these helper methods use direct string interpolation to build SQL, they are designed for internal migration use with developer-controlled inputs. However, if these helpers are ever used with dynamic or untrusted data, SQL injection could occur.
Consider documenting the assumption that inputs must be trusted/sanitized, or add basic escaping for defense in depth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/config/APIEndpointsMigrationHelper.php` around lines 48 - 68, The insertEndpoint helper builds a raw SQL string using unescaped interpolated parameters (apiName, endpointName, route, httpMethod) which could allow SQL injection if used with untrusted data; update the insertEndpoint method to either (1) explicitly document in the docblock that all inputs must be trusted/sanitized and intended only for developer-controlled migration data, or (2) implement basic escaping/sanitization for these parameters (or switch to parameterized queries) before interpolation; reference the insertEndpoint method name when making the change and ensure the chosen approach is applied consistently to similar helpers in this file for defense-in-depth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php`:
- Around line 3423-3429: The security annotation for the addExtraQuestion
endpoint in OAuth2SummitSponsorApiController.php is missing the new
WriteSponsorExtraQuestions scope; update the security array used by the
addExtraQuestion method to include SummitScopes::WriteSponsorExtraQuestions
alongside SummitScopes::WriteSummitData so the endpoint matches the migration
and other write endpoints (e.g., updateExtraQuestion, deleteExtraQuestion,
addExtraQuestionValue).
---
Nitpick comments:
In @.gitignore:
- Line 42: The .gitignore currently ignores the entire docs/ tree which hides
source doc changes; update the ignore entry to target only generated artifacts
(e.g., replace or remove the generic "docs/" entry and add a specific path like
"docs/build/" or "docs/_site/" as appropriate) so documentation source files
remain tracked; ensure you update the .gitignore entry that currently contains
"docs/" to the specific generated-directory name used by your build tooling.
In `@database/migrations/config/APIEndpointsMigrationHelper.php`:
- Around line 48-68: The insertEndpoint helper builds a raw SQL string using
unescaped interpolated parameters (apiName, endpointName, route, httpMethod)
which could allow SQL injection if used with untrusted data; update the
insertEndpoint method to either (1) explicitly document in the docblock that all
inputs must be trusted/sanitized and intended only for developer-controlled
migration data, or (2) implement basic escaping/sanitization for these
parameters (or switch to parameterized queries) before interpolation; reference
the insertEndpoint method name when making the change and ensure the chosen
approach is applied consistently to similar helpers in this file for
defense-in-depth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bf93eab-b43a-411a-b61f-f43200f6888f
📒 Files selected for processing (9)
.gitignoreapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.phpapp/Security/SummitScopes.phpapp/Swagger/Security/SponsorOAuth2Schema.phpdatabase/migrations/config/APIEndpointsMigrationHelper.phpdatabase/migrations/config/Version20260408143000.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ApiScopesSeeder.phproutes/api_v1.php
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-524/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
database/migrations/config/APIEndpointsMigrationHelper.php (1)
34-68: Consider SQL escaping for future-proofing.The helper methods use direct string interpolation (
'{$endpointName}') without escaping. While current usage passes compile-time constants (e.g.,SummitScopes::ReadSponsorExtraQuestions), this pattern could be fragile if future callers pass dynamic values containing special SQL characters.Consider using
addslashes()or a more robust escaping mechanism:$escapedName = addslashes($endpointName); // then use '{$escapedName}' in SQLAlternatively, Doctrine's
addSql()supports parameterized queries as a second argument, though this requires restructuring the helper return values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/config/APIEndpointsMigrationHelper.php` around lines 34 - 68, The insertEndpoint helper currently injects raw strings into the SQL (see insertEndpoint parameters $apiName, $endpointName, $route, $httpMethod), which risks SQL syntax issues if callers pass dynamic values; update insertEndpoint to escape those values before interpolation (e.g., compute $escapedApi = addslashes($apiName), $escapedName = addslashes($endpointName), $escapedRoute = addslashes($route), $escapedMethod = addslashes($httpMethod) and use those escaped variables in the returned SQL) or refactor to return a parameterized statement compatible with the migration runner (so callers can pass parameters to addSql), ensuring all four identifiers are sanitized consistently within the method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@database/migrations/config/APIEndpointsMigrationHelper.php`:
- Around line 48-68: The insertEndpoint helper inserts http_method as a plain
string even though the schema (see Version20190422160409.php) defines
http_method as an array/serialized field; update insertEndpoint to accept the
HTTP methods as an array or a pre-serialized string and ensure it serializes
arrays the same way the seeder/Doctrine does (or validate caller-provided
serialization), i.e., change the signature/use of insertEndpoint (and the local
variable handling for $httpMethod) so that when given an array it converts it to
the correct serialized format used by the db, or document that callers must pass
a pre-serialized $httpMethod string.
---
Nitpick comments:
In `@database/migrations/config/APIEndpointsMigrationHelper.php`:
- Around line 34-68: The insertEndpoint helper currently injects raw strings
into the SQL (see insertEndpoint parameters $apiName, $endpointName, $route,
$httpMethod), which risks SQL syntax issues if callers pass dynamic values;
update insertEndpoint to escape those values before interpolation (e.g., compute
$escapedApi = addslashes($apiName), $escapedName = addslashes($endpointName),
$escapedRoute = addslashes($route), $escapedMethod = addslashes($httpMethod) and
use those escaped variables in the returned SQL) or refactor to return a
parameterized statement compatible with the migration runner (so callers can
pass parameters to addSql), ensuring all four identifiers are sanitized
consistently within the method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 931e5071-e7a4-4b99-b251-b6628af64bf3
📒 Files selected for processing (3)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.phpdatabase/migrations/config/APIEndpointsMigrationHelper.phpdatabase/migrations/config/Version20260408143000.php
| protected function insertEndpoint( | ||
| string $apiName, | ||
| string $endpointName, | ||
| string $route, | ||
| string $httpMethod, | ||
| bool $active = true, | ||
| bool $allowCors = true, | ||
| bool $allowCredentials = true | ||
| ): string { | ||
| $activeInt = $active ? 1 : 0; | ||
| $corsInt = $allowCors ? 1 : 0; | ||
| $credentialsInt = $allowCredentials ? 1 : 0; | ||
|
|
||
| return <<<SQL | ||
| INSERT INTO api_endpoints (api_id, name, route, http_method, active, allow_cors, allow_credentials, created_at, updated_at) | ||
| SELECT a.id, '{$endpointName}', '{$route}', '{$httpMethod}', {$activeInt}, {$corsInt}, {$credentialsInt}, NOW(), NOW() | ||
| FROM apis a | ||
| WHERE a.name = '{$apiName}' | ||
| AND NOT EXISTS (SELECT 1 FROM api_endpoints e WHERE e.api_id = a.id AND e.name = '{$endpointName}'); | ||
| SQL; | ||
| } |
There was a problem hiding this comment.
http_method column type mismatch in insertEndpoint().
The schema defines http_method as an array type (per Version20190422160409.php), but this method inserts it as a plain string. While this method isn't used in the current migration, future usage could cause data type issues.
The seeder likely uses Doctrine's array serialization. If this helper is intended for use, consider matching the seeder's approach or documenting that this method requires the caller to pre-serialize the HTTP method array.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@database/migrations/config/APIEndpointsMigrationHelper.php` around lines 48 -
68, The insertEndpoint helper inserts http_method as a plain string even though
the schema (see Version20190422160409.php) defines http_method as an
array/serialized field; update insertEndpoint to accept the HTTP methods as an
array or a pre-serialized string and ensure it serializes arrays the same way
the seeder/Doctrine does (or validate caller-provided serialization), i.e.,
change the signature/use of insertEndpoint (and the local variable handling for
$httpMethod) so that when given an array it converts it to the correct
serialized format used by the db, or document that callers must pass a
pre-serialized $httpMethod string.
ref: https://app.clickup.com/t/86b94xa4t
Summary by CodeRabbit
New Features
Chores