fix: keep optional credentials in location and OAuth URLs - #1911
HarshMN2345 wants to merge 1 commit into
Conversation
Location and OAuth methods build their URL from the same schemes as examples. Examples now skip schemes that security marks optional, so impersonation was also dropped from download, preview and avatar URLs and the request ran as the caller instead of the impersonated user. Add a urlSecuritySchemes filter that keeps optional schemes and use it wherever a template embeds credentials into a URL. Examples still skip them.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| {% endmacro %} | ||
| {% macro methodNeedsSecurityParameters(method) %} | ||
| {% if ((method | methodType) == "webAuth" or (method | methodType) == "location") and [(method | securitySchemes)]|length > 0 %}{{ true }}{% else %}{{false}}{% endif %} | ||
| {% if ((method | methodType) == "webAuth" or (method | methodType) == "location") and [(method | urlSecuritySchemes)]|length > 0 %}{{ true }}{% else %}{{false}}{% endif %} |
There was a problem hiding this comment.
methodNeedsSecurityParameters, but that macro has no callers. The code that actually builds parameters for location and OAuth requests still does not use urlSecuritySchemes. Generated .NET URLs therefore continue to omit optional credentials such as impersonateuserid, so impersonated requests can run as the caller and fail authorization. The new filter must be used by the parameter-building path consumed by location.twig and oauth.twig.
Prompt To Fix With AI
This is a comment left during a code review.
Path: templates/dotnet/base/utils.twig
Line: 12
Comment:
The .NET change updates only `methodNeedsSecurityParameters`, but that macro has no callers. The code that actually builds parameters for location and OAuth requests still does not use `urlSecuritySchemes`. Generated .NET URLs therefore continue to omit optional credentials such as `impersonateuserid`, so impersonated requests can run as the caller and fail authorization. The new filter must be used by the parameter-building path consumed by `location.twig` and `oauth.twig`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| $this->twig->addFilter(new TwigFilter('methodHeaders', fn(Operation $operation): array => $this->getMethodHeaders($operation))); | ||
| $this->twig->addFilter(new TwigFilter('responseDiscriminator', fn(Operation $operation): array => $this->getResponseDiscriminator($operation))); | ||
| $this->twig->addFilter(new TwigFilter('securitySchemes', fn(Operation $operation): array => $this->getOperationAuthSchemes($operation))); | ||
| $this->twig->addFilter(new TwigFilter('urlSecuritySchemes', fn(Operation $operation): array => $this->getOperationAuthSchemes($operation, optional: true))); |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/SDK/SDK.php
Line: 188
Comment:
This new filter and its template integrations change generated URL behavior without an observable regression test. The existing security-alternative test protects example generation, but it does not verify that location and OAuth output includes optional credentials while examples exclude them. That gap allowed the ineffective .NET integration to go unnoticed. Please add a generation test that renders representative optional security alternatives and checks the generated URL behavior across the affected template families.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
What does this PR do?
Since #1891,
securitySchemesskips schemes thatsecurityonly accepts as an alternative. That is right for examples, but the web, React Native, Dart, Flutter, Swift, Android and .NET templates also use this filter to put client credentials into location and OAuth URLs. The Appwrite spec now listsImpersonateUserIdonly in the second security alternative for location methods, so regenerated SDKs stopped addingimpersonateuseridto those URLs. A console user who is impersonating someone and downloads a deployment, previews a file or loads an avatar sends the request as themselves and gets a 401.This PR adds a
urlSecuritySchemesfilter that keeps optional schemes and uses it in the templates that build these URLs. Examples still skip them.Test Plan
web consolefrom the publishedopen-api3-latest.jsonbefore and after the change. The only difference is 16 restoredpayload['impersonateuserid'] = this.client.config.impersonateuserid;lines in avatars, functions, organizations, sites and storage. This matches the console SDK built before feat: derive optional security from OpenAPI alternatives #1891. Docs examples are unchanged.react-native clientgets back the same line in avatars and storage.flutter,androidandappleclient output is unchanged.format:check,lintandanalysepass for the generated web and React Native SDKs.tests/generation/GenerationTest.phppasses, includingtestExamplesRespectSecurityAlternatives. phpcs, rector and djlint pass on the changed files.impersonateuseridreturns 200 for an impersonator. Without it the same request returns 401.