Update tokens without JWT #9750
Conversation
WalkthroughThis update refactors resource token handling by removing permissions management, tightening admin-only access on token endpoints, and simplifying token creation and update logic. JWT token retrieval is now integrated into the token model's response, eliminating a separate endpoint. Error handling is improved with explicit checks for invalid resource types, and related tests are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant TokenModel
participant JWTEncoder
Client->>API: Create resource token
API->>TokenModel: Generate token document
TokenModel->>JWTEncoder: Encode JWT with token attributes
JWTEncoder-->>TokenModel: JWT string
TokenModel-->>API: Token document with JWT in 'secret'
API-->>Client: Token response (includes JWT)
Client->>API: Use token (e.g., for file preview)
API->>TokenModel: Validate token (resource type, expiry)
TokenModel-->>API: Validation result
API-->>Client: File preview/download/view
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
app/controllers/api/storage.php
Outdated
| @@ -1284,13 +1288,15 @@ | |||
| )) | |||
| ->param('bucketId', '', new UID(), 'Storage bucket unique ID. You can create a new storage bucket using the Storage service [server integration](https://appwrite.io/docs/server/storage#createBucket).') | |||
| ->param('fileId', '', new UID(), 'File ID.') | |||
| // NOTE: this is only for the sdk generator and is not used in the action below. | |||
There was a problem hiding this comment.
In notes, lets metion token param is used by resource (resource name, file name)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Appwrite/Utopia/Response/Model/ResourceToken.php (1)
69-93: Good implementation of JWT token generation.The new
filtermethod handles token expiration calculation and JWT encoding properly. The max age logic (capping expired tokens at 360 seconds) is sensible and the JWT encoding includes all necessary resource token attributes.Consider using a named constant instead of the hardcoded
360value at line 79 to improve maintainability and document the purpose of this specific duration.app/controllers/api/storage.php (1)
954-1292: Consider documenting the SDK parameter generation pattern.Since the token parameters are only for SDK generation and not used in the action implementation, consider adding more formal documentation explaining this pattern to help developers understand why these parameters exist but are unused in the implementation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/config/errors.php(1 hunks)app/controllers/api/storage.php(3 hunks)app/init/resources.php(1 hunks)src/Appwrite/Extend/Exception.php(1 hunks)src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Create.php(1 hunks)src/Appwrite/Platform/Modules/Tokens/Http/Tokens/JWT/Get.php(0 hunks)src/Appwrite/Platform/Modules/Tokens/Services/Http.php(0 hunks)src/Appwrite/Utopia/Response/Model/ResourceToken.php(3 hunks)tests/e2e/Services/Tokens/TokensBase.php(2 hunks)
💤 Files with no reviewable changes (2)
- src/Appwrite/Platform/Modules/Tokens/Services/Http.php
- src/Appwrite/Platform/Modules/Tokens/Http/Tokens/JWT/Get.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/config/errors.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-406)
app/init/resources.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-406)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (11)
src/Appwrite/Extend/Exception.php (1)
325-325: Added new error constant for invalid token resource typesThis new constant will be used to throw a more specific error when a token is created with an invalid resource type, improving error handling and client feedback in the updated token management system.
app/config/errors.php (1)
502-506: Well-defined error mapping for invalid token resource typesThe new error definition properly maps the
TOKEN_RESOURCE_TYPE_INVALIDconstant to an HTTP 400 status code with a clear description. This enables better error reporting when invalid token resource types are encountered during validation.src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Create.php (1)
74-74: Added explicit void return type declarationAdding the explicit
voidreturn type improves code clarity and type safety. It makes it clear that this method doesn't return any values, which aligns with its implementation.tests/e2e/Services/Tokens/TokensBase.php (2)
69-69: Return entire token object instead of just the IDReturning the complete token object enables direct access to all token properties, supporting the simplified JWT retrieval approach that no longer requires a separate API call.
151-151: Simplified JWT token retrievalDirectly using the token's
secretfield instead of making a separate API call to retrieve the JWT is a more efficient approach. This change reflects the architectural improvement where JWTs are now generated by the token model itself.src/Appwrite/Utopia/Response/Model/ResourceToken.php (1)
53-59: Newsecretattribute implementation looks good.The new attribute for storing JWT encoded tokens is well-documented with clear type and description.
app/init/resources.php (2)
931-940: Improved expiry handling logic.The expiry check is now performed earlier in the flow and uses clearer datetime comparison.
942-966: Good refactoring to use match expression for resource type validation.Using a match expression makes the code more maintainable and explicitly handles different resource types with clear error paths. This will make it easier to add additional resource types in the future.
The explicit exception for invalid token resource types provides better error handling than the previous approach.
app/controllers/api/storage.php (3)
954-955: Token parameter addition for SDK generation.Adding the optional token parameter to the file preview endpoint supports the new token handling approach, even though it's not used directly in the action implementation.
1134-1135: Token parameter addition for SDK generation.Adding the optional token parameter to the file download endpoint supports the new token handling approach, even though it's not used directly in the action implementation.
1291-1292: Token parameter addition for SDK generation.Adding the optional token parameter to the file view endpoint supports the new token handling approach, even though it's not used directly in the action implementation.
| ]); | ||
| })(), | ||
|
|
||
| default => throw new Exception(Exception::TOKEN_RESOURCE_TYPE_INVALID), |
There was a problem hiding this comment.
should we throw error here, or return empty document like in other cases?
There was a problem hiding this comment.
I think it makes sense to keep it a bit stricter. add case by case and throw an error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Utopia/Response/Model/ResourceToken.php (1)
69-93: Consider adding error handling for JWT encoding.The JWT encoding process doesn't include any error handling. Although JWT encoding rarely fails, it's a good practice to add try-catch blocks around external library calls, especially when they're critical to the application's functionality.
public function filter(Document $document): Document { $maxAge = PHP_INT_MAX; $expire = $document->getAttribute('expire'); if ($expire !== null) { $now = new \DateTime(); $expiryDate = new \DateTime($expire); // set 1 min if expired, we check for expiry later on route hooks for validation! $maxAge = min(60, $expiryDate->getTimestamp() - $now->getTimestamp()); } $jwt = new JWT(System::getEnv('_APP_OPENSSL_KEY_V1'), 'HS256', $maxAge, 10); - $secret = $jwt->encode([ - 'tokenId' => $document->getId(), - 'resourceId' => $document->getAttribute('resourceId'), - 'resourceType' => $document->getAttribute('resourceType'), - 'resourceInternalId' => $document->getAttribute('resourceInternalId'), - ]); + try { + $secret = $jwt->encode([ + 'tokenId' => $document->getId(), + 'resourceId' => $document->getAttribute('resourceId'), + 'resourceType' => $document->getAttribute('resourceType'), + 'resourceInternalId' => $document->getAttribute('resourceInternalId'), + ]); + } catch (\Exception $e) { + // Log the error + throw new \Exception('Failed to generate token: ' . $e->getMessage()); + } $document->setAttribute('secret', $secret); return $document; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/storage.php(3 hunks)src/Appwrite/Utopia/Response/Model/ResourceToken.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/storage.php
🧰 Additional context used
🪛 Gitleaks (8.21.2)
src/Appwrite/Utopia/Response/Model/ResourceToken.php
58-58: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Benchmark
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (4)
src/Appwrite/Utopia/Response/Model/ResourceToken.php (4)
53-59: Good addition of the secret field to encapsulate the JWT token.The addition of the secret field to store the JWT token directly in the resource token model is a good design choice. This allows clients to access the JWT without making an additional API call.
🧰 Tools
🪛 Gitleaks (8.21.2)
58-58: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
82-88: Good implementation of JWT encoding.The JWT generation properly uses the HS256 algorithm with a server-side secret key from environment variables. The token payload contains only the necessary information (tokenId, resourceId, resourceType, resourceInternalId) without exposing permissions or other sensitive data.
58-58: No security risk from example JWT.The Gitleaks static analysis warning about a JWT in the example is a false positive. This is properly used as documentation and doesn't expose any real secrets.
🧰 Tools
🪛 Gitleaks (8.21.2)
58-58: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
79-79:❌ Incorrect review comment
Verify the expired token handling.
Setting a 1-minute max age for expired tokens seems questionable from a security perspective. While your comment indicates the expiry is checked later in route hooks, consider if this grace period is necessary and secure.
🏁 Script executed:
#!/bin/bash # Checking where the token expiry is validated in route hooks rg -A 2 -B 2 "token.*expir|expir.*token" --type phpLength of output: 8082
Cookie maxAge cap is for sliding sessions, not expiry grace period
The 60-second cap in
ResourceToken.phponly limits the Set-Cookiemax-ageto enable a sliding session refresh—it is not a post-expiry grace window. Actual token expiration is enforced inAppwrite\Auth\Auth::action()via theDateTime::formatTz($token->getAttribute('expire')) >= DateTime::formatTz(DateTime::now())check. No changes needed here.Likely an incorrect or invalid review comment.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/config/collections/projects.php (1)
2085-2086: Consider renaming the index to reflect its new attribute.The index attribute has been correctly updated from
functionInternalIdtoresourceInternalIdas part of the broader token handling refactoring. However, the index name_key_function_internal_idno longer reflects what's being indexed, which could confuse developers.Consider renaming the index to match its new purpose:
- '$id' => ID::custom('_key_function_internal_id'), + '$id' => ID::custom('_key_resource_internal_id'),This would maintain consistency with the naming conventions seen elsewhere in the file, such as the
_key_resource_internal_id_resource_typeindex at line 2219.src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Update.php (2)
48-50: Docstring still mentions “update a token’s expiry date” only – clarify full behaviourThe description now states you can only update the expiry date.
If that limitation is intentional, consider making it explicit in the sentence (e.g. “Only theexpirefield can be modified.”) so SDK docs are crystal-clear.
60-60: Stale parameter wording – references “File token”
->param('expire', … 'File token expiry date', true)still carries the legacy “File token” phrasing.
Suggest removing “File” to avoid confusion now that tokens are generic.- ->param('expire', null, new Nullable(new DatetimeValidator()), 'File token expiry date', true) + ->param('expire', null, new Nullable(new DatetimeValidator()), 'Token expiry date.', true)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
app/config/collections/platform.php(1 hunks)app/config/collections/projects.php(1 hunks)src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Create.php(2 hunks)src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Delete.php(1 hunks)src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Get.php(1 hunks)src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Update.php(2 hunks)src/Appwrite/Utopia/Response/Model/ResourceToken.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Utopia/Response/Model/ResourceToken.php
🔇 Additional comments (6)
src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Get.php (1)
41-41: Breaking-change in auth requirements – make sure callers & docs are updatedChanging the SDK
autharray to[AuthType::ADMIN]prevents sessions, API keys and JWTs from hitting/v1/tokens/:tokenId.
This will break existing client-side integrations that relied on those auth methods.Action items
• Update public docs / changelog to mention the new admin-only restriction.
• Check SDK generation – older SDKs may still advertise the removed auth types.
• Scan the codebase/tests for non-admin calls togetTokenand adjust them.src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Delete.php (1)
48-49: Auth tightened to admin-only – verify downstream impactAs with the Get and Update endpoints, deletion now demands
AuthType::ADMIN.
Please confirm:• Client libraries regenerate with the new requirement.
• Any automated clean-up jobs or user-level dashboards that used API keys are adapted.src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Create.php (4)
51-51: Documentation update looks good.The description has been updated to be more precise about token linking, clearly stating that a token is linked to a file. This improves clarity for API consumers.
71-71: Simplified method signature aligns with token system refactoring.The permissions parameter has been removed from the method signature as part of the broader effort to simplify token creation by removing permissions handling. This change maintains consistency with the overall token system refactoring.
93-100: Token creation successfully simplified.The token creation process has been properly simplified by removing the permissions field while maintaining all essential attributes (id, secret, resource identifiers, type, and expiration). The security checks on lines 80-91 ensure proper authorization is still enforced.
51-112:❓ Verification inconclusive
Verify impact on API consumers.
The removal of permissions handling from token creation represents a significant architectural change. While the implementation looks correct, it's important to verify that API consumers are aware of this change and its implications.
🏁 Script executed:
#!/bin/bash # Check if there are any API clients or SDK implementations that might be affected by this change # Look for any usage of the tokens API with permissions parameter rg -i "createFileToken.*permissions" --type ts --type js --type php # Check if this change is documented in any changelog or migration guide rg -i "token.*permissions" --glob "CHANGELOG.md" rg -i "token.*permissions" --glob "*.md" --glob "!node_modules"Length of output: 333
Ensure API consumers are informed of removed permissions handling
Although a search didn’t reveal any client or documentation references to a
permissionsparameter in this repo, removing permissions logic from the Create File Token endpoint is a breaking change. Please verify and update accordingly:
- Review all client SDKs (TS/JS/PHP/etc.) for any
createFileToken(…, permissions)signatures and adjust or deprecate them.- Update CHANGELOG.md and any migration guides to call out the removed parameter and new authorization checks.
- Refresh official docs (appwrite.io) for the Create File Token API to remove/replace any mention of
permissions.- Communicate this change to API consumers (e.g., via release notes or blog post).
commit: |
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist
Summary by CodeRabbit
New Features
Changes
Bug Fixes
Tests