Add validity time option to the API (DIS-2749) #299
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
paulgb
changed the title
paulgb
force-pushed
the
paulgb/add-validity-time-option-to-api
branch
from
de27c86
to
bc4f3e1
Compare
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes across multiple files in the Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🔇 Additional comments (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (5)
js-pkg/sdk/src/types.ts (1)
34-46: LGTM: Well-structuredAuthDocRequesttype with a minor suggestion.The
AuthDocRequesttype is well-defined and aligns perfectly with the PR objectives. It includes the crucialvalidForSecondsproperty for specifying token validity time, and the optional nature of all properties provides great flexibility.Consider improving type safety for the
metadataproperty:metadata?: Record<string, unknown>;This change ensures that any value can be stored in the metadata while maintaining type safety.
crates/y-sweet-core/src/api_types.rs (1)
37-39: LGTM! Consider adding documentation for the new field.The addition of the
valid_for_secondsfield aligns well with the PR objective of allowing specification of token validity time. The use ofOption<u64>and the serde annotations are appropriate.Consider adding a doc comment to explain the purpose and expected format of the
valid_for_secondsfield. For example:/// The duration in seconds for which the authentication token should be valid. /// If not specified, a default value will be used. #[serde(skip_serializing_if = "Option::is_none", rename = "validForSeconds")] pub valid_for_seconds: Option<u64>,crates/y-sweet/src/server.rs (2)
654-658: Ensure consistent naming and remove debug printThe implementation for handling the
valid_for_secondsparameter looks good. However, there are a couple of suggestions:
- Consider renaming
valid_for_secondstoexpiration_time_secondsfor consistency with theExpirationTimeEpochMillistype.- Remove the debug print statement on line 655, as it's not necessary for production code.
Here's a suggested improvement:
-let valid_for_seconds = body.valid_for_seconds.unwrap_or(DEFAULT_EXPIRATION_MILLIS); -println!("valid_for_seconds {}", valid_for_seconds); -let expiration_time = - ExpirationTimeEpochMillis(current_time_epoch_millis() + valid_for_seconds * 1000); +let expiration_time_seconds = body.valid_for_seconds.unwrap_or(DEFAULT_EXPIRATION_MILLIS); +let expiration_time = ExpirationTimeEpochMillis( + current_time_epoch_millis() + expiration_time_seconds * 1000 +);
756-756: LGTM: Test case for default behaviorPassing
Noneinstead of aJson<AuthDocRequest>is a good test for the default behavior when no request body is provided. This helps ensure backward compatibility.Consider adding an additional test case that explicitly sets a custom
valid_for_secondsvalue to ensure that the expiration time is calculated correctly.crates/y-sweet-core/src/auth.rs (1)
13-13: Typo in documentation commentThere's a typo in the comment on line 13: "intentonally" should be "intentionally".
Apply this diff to correct the typo:
-/// We introduce this to intentonally break callers to `gen_doc_token` that do not explicitly +/// We introduce this to intentionally break callers to `gen_doc_token` that do not explicitly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- crates/y-sweet-core/src/api_types.rs (1 hunks)
- crates/y-sweet-core/src/auth.rs (10 hunks)
- crates/y-sweet-worker/src/lib.rs (3 hunks)
- crates/y-sweet/src/server.rs (4 hunks)
- js-pkg/sdk/src/main.ts (6 hunks)
- js-pkg/sdk/src/types.ts (1 hunks)
- tests/src/index.test.ts (2 hunks)
🧰 Additional context used
🪛 Biome
tests/src/index.test.ts
[error] 299-299: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
🔇 Additional comments (17)
js-pkg/sdk/src/types.ts (2)
32-32: LGTM: Clear and conciseAuthorizationtype definition.The
Authorizationtype is well-defined as a union of 'full' and 'read-only'. This aligns with the PR objectives of enhancing token management flexibility.
31-46: Summary: New type definitions align well with PR objectives.The additions of
AuthorizationandAuthDocRequesttypes are well-implemented and directly support the PR's goal of enhancing token management flexibility. ThevalidForSecondsproperty inAuthDocRequestaddresses the main objective of allowing specification of token validity time. These changes provide a solid foundation for implementing the new features described in the PR objectives.crates/y-sweet-core/src/api_types.rs (1)
41-49: LGTM! Verify the default authorization level.The
Defaultimplementation forAuthDocRequestis well-structured and includes all fields, including the newvalid_for_secondsfield.Please confirm that
Authorization::Fullis the intended default authorization level. If it's correct, consider adding a comment explaining why full authorization is the default. If not, adjust accordingly.To verify the usage of
Authorization::Fullacross the codebase, you can run the following script:This will help ensure consistency in the default authorization level across the project.
crates/y-sweet/src/server.rs (4)
35-35: LGTM: Appropriate imports addedThe addition of
ExpirationTimeEpochMillisandDEFAULT_EXPIRATION_MILLISto the imports is consistent with their usage in the updatedauth_docfunction. This change follows good practices by importing specific items.
644-644: LGTM: Improved function signature for backward compatibilityThe updated function signature now accepts an optional
Json<AuthDocRequest>, which is then unwrapped with a default value. This change allows for backward compatibility while introducing the new functionality to specify token validity time.Also applies to: 648-649
718-723: LGTM: Updated test case for new functionalityThe test case has been appropriately updated to include the new
valid_for_secondsfield in theAuthDocRequest. This ensures that the new functionality is being tested.
Line range hint
1-765: Overall assessment: Well-implemented feature with minor suggestionsThe changes successfully implement the new feature to allow specifying token validity time while maintaining backward compatibility. The code is well-structured and the test cases have been appropriately updated. Here's a summary of the suggestions:
- Consider renaming
valid_for_secondstoexpiration_time_secondsfor consistency.- Remove the debug print statement in the
auth_docfunction.- Add an additional test case that explicitly sets a custom
valid_for_secondsvalue.These minor improvements will enhance the code quality and test coverage. Great work overall!
crates/y-sweet-worker/src/lib.rs (3)
11-14: Imports updated correctlyThe necessary types and constants have been successfully imported to support the new functionality.
194-194: Function signature updated appropriatelyThe
auth_docfunction now accepts mutablereqandctx, which is necessary for parsing the request body and modifying the context.
220-220: Ensure correct usage ofgen_doc_tokenwith updated expiration time
auth.gen_doc_tokenis now called withexpiration_time, ensuring tokens are generated with the specified validity period.tests/src/index.test.ts (1)
2-2: ImportingYSweetErrorfor error handlingThe addition of
YSweetErrorin the import statement is appropriate, as it facilitates proper error handling in the new test cases.crates/y-sweet-core/src/auth.rs (6)
9-10: Renaming and exposingDEFAULT_EXPIRATION_MILLISimproves clarity and accessibilityRenaming
EXPIRATION_MILLIStoDEFAULT_EXPIRATION_MILLISclarifies its purpose as the default expiration time for tokens. Making itpuballows other modules to access this constant as needed, aligning with the PR objectives of enhancing token management flexibility.
20-24:ExpirationTimeEpochMillis::max()provides a clear way to represent infinite expirationImplementing the
max()method enhances usability by offering a straightforward way to represent tokens with no specific expiration time.
102-102: Updatingexpiration_millisto useExpirationTimeEpochMillisenhances type safetyChanging the
expiration_millisfield toOption<ExpirationTimeEpochMillis>ensures that expiration times are explicitly and safely handled, reducing the risk of errors associated with rawu64values.
159-162:Payload::new_with_expirationnow requiresExpirationTimeEpochMillisfor clarityBy accepting
ExpirationTimeEpochMillis, the constructor enforces explicit specification of expiration times, enhancing readability and reducing potential misuse.
277-283: Properly handling token expiration usingExpirationTimeEpochMillisThe updated logic correctly checks token expiration by comparing the expiration time with the current time, accounting for tokens without a set expiration by defaulting to
ExpirationTimeEpochMillis::max().
316-322:gen_doc_tokennow requires explicit expiration timeRequiring an
ExpirationTimeEpochMillisparameter forgen_doc_tokenenforces that callers explicitly specify the token's expiration, improving security and aligning with the PR's goals to make token validity configurable.
| public async getClientToken( | ||
| docId: string | DocCreationResult, | ||
| authDocRequest?: AuthDocRequest, | ||
| ): Promise<ClientToken> { |
There was a problem hiding this comment.
Update JSDoc comments to include the 'authDocRequest' parameter
The getClientToken method now accepts an optional authDocRequest parameter, but the JSDoc comments above the method do not reflect this change. Please update the documentation to include a description of authDocRequest and its purpose.
| public async getDocConnection( | ||
| docId: string | DocCreationResult, | ||
| authDocRequest?: AuthDocRequest, | ||
| ): Promise<DocConnection> { |
There was a problem hiding this comment.
Add documentation for the new 'authDocRequest' parameter
The getDocConnection method signature now includes an optional authDocRequest parameter, but the accompanying JSDoc comments have not been updated. Please provide a description of authDocRequest in the documentation to clarify its function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js-pkg/sdk/src/main.ts (1)
131-135: Update JSDoc forgetDocConnectionmethod.The
getDocConnectionmethod has been correctly updated to include theauthDocRequestparameter and pass it togetClientToken. However, the JSDoc comments for this method have not been updated to reflect this change.Please update the JSDoc to include a description of the
authDocRequestparameter, similar to the other methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- js-pkg/sdk/src/main.ts (5 hunks)
🧰 Additional context used
🔇 Additional comments (6)
js-pkg/sdk/src/main.ts (6)
4-4: LGTM: Import statement updated correctly.The import statement has been properly updated to include the
AuthDocRequesttype, which is necessary for the new parameter added to various methods.
73-79: LGTM:getClientTokenmethod updated correctly.The
getClientTokenmethod has been properly updated:
- The method signature now includes the optional
authDocRequestparameter.- The JSDoc has been updated to describe the new parameter, addressing the previous review comment.
- The request body in the implementation now correctly uses the
authDocRequestparameter.These changes improve the flexibility of the authentication process as intended.
Also applies to: 84-84
98-104: LGTM:getOrCreateDocAndTokenmethod updated correctly.The
getOrCreateDocAndTokenmethod has been properly updated:
- The method signature now includes the optional
authDocRequestparameter.- The JSDoc has been updated to describe the new parameter, addressing the previous review comment.
- The implementation now correctly passes the
authDocRequestto thegetClientTokenmethod.These changes ensure that the authentication request options are properly propagated.
Also applies to: 106-106
146-146: LGTM:getOrCreateDocAndTokenfunction updated correctly.The
getOrCreateDocAndTokenfunction has been properly updated:
- The function signature now includes the optional
authDocRequestparameter.- The JSDoc has been updated to describe the new parameter, addressing the previous review comment.
- The implementation now correctly passes the
authDocRequestto theDocumentManager.getOrCreateDocAndTokenmethod.These changes ensure that the authentication request options are properly propagated through the convenience function.
Also applies to: 152-152, 155-155
163-163: LGTM:getClientTokenfunction updated correctly.The
getClientTokenfunction has been properly updated:
- The function signature now includes the optional
authDocRequestparameter.- The JSDoc has been updated to describe the new parameter, addressing the previous review comment.
- The implementation now correctly passes the
authDocRequestto theDocumentManager.getClientTokenmethod.These changes ensure that the authentication request options are properly propagated through the convenience function.
Also applies to: 169-169, 172-172
Line range hint
1-189: Overall changes align well with PR objectives.The changes in this file successfully implement the new feature allowing specification of validity time for authentication tokens through the
authDocRequestparameter. The modifications are consistent across different methods and functions, ensuring that the new parameter is properly propagated throughout the authentication process.Key points:
- The
AuthDocRequesttype is correctly imported and used.- All relevant methods and functions (
getClientToken,getOrCreateDocAndToken,getDocConnection) have been updated to include the new parameter.- JSDoc comments have been updated in most places to reflect the changes (except for
getDocConnection, which has been addressed in a previous comment).- The changes align well with the PR objectives and the AI-generated summary.
These updates enhance the flexibility of token management as intended, allowing users to define how long a token remains valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
crates/y-sweet-core/src/auth.rs (5)
9-10: LGTM! Consider adding a doc comment.The introduction of
DEFAULT_EXPIRATION_SECONDSas a public constant is a good change. It aligns with the PR objective of making token validity time configurable and uses a more intuitive time unit (seconds).Consider adding a doc comment to explain the purpose and usage of this constant, especially since it's now public. For example:
/// Default expiration time for authentication tokens in seconds. /// This value is used when no specific expiration time is provided. pub const DEFAULT_EXPIRATION_SECONDS: u64 = 60 * 60; // 60 minutes
11-18: LGTM! Consider implementing additional traits.The introduction of
ExpirationTimeEpochMillisis a good change that improves type safety and prevents potential misuse of thegen_doc_tokenfunction. The derived traits and the explanatory comment are appropriate.Consider implementing additional traits that might be useful for this type:
use std::ops::Deref; #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)] pub struct ExpirationTimeEpochMillis(pub u64); impl Deref for ExpirationTimeEpochMillis { type Target = u64; fn deref(&self) -> &Self::Target { &self.0 } }This would allow for easier comparison and dereferencing to the underlying
u64value when needed.
20-24: LGTM! Consider adding a doc comment.The
max()method is a good addition, providing a convenient way to represent an "infinite" expiration time.Consider adding a doc comment to explain the purpose and usage of this method:
impl ExpirationTimeEpochMillis { /// Returns the maximum possible expiration time, effectively representing "never expires". pub fn max() -> Self { Self(u64::MAX) } }
316-322: LGTM! Consider adding a default expiration option.The changes to
gen_doc_tokencorrectly incorporate the newExpirationTimeEpochMillistype, improving type safety and preventing accidental misuse with rawu64values.Consider adding an overloaded method or a default parameter to allow users to generate tokens with the default expiration time easily:
impl Authenticator { // ... existing methods ... pub fn gen_doc_token_with_default_expiration(&self, doc_id: &str) -> String { self.gen_doc_token(doc_id, ExpirationTimeEpochMillis(DEFAULT_EXPIRATION_SECONDS * 1000)) } }This would provide a convenient way for users to generate tokens without specifying an expiration time, while still allowing custom expiration times when needed.
389-392: LGTM! Consider adding more test cases.The updates to the test cases correctly incorporate the new
ExpirationTimeEpochMillistype and make use of theDEFAULT_EXPIRATION_SECONDSconstant. These changes ensure that the tests remain consistent with the main code and improve maintainability.Consider adding more test cases to cover different scenarios:
- Test with a custom expiration time that's neither 0 nor the default.
- Test the behavior when the current time is exactly at the expiration time.
- Test with
ExpirationTimeEpochMillis::max()to ensure "never expires" behavior works as expected.For example:
#[test] fn test_custom_expiration_time() { let authenticator = Authenticator::gen_key().unwrap(); let custom_expiration = 3600; // 1 hour let token = authenticator.gen_doc_token("doc123", ExpirationTimeEpochMillis(custom_expiration * 1000)); assert_eq!(authenticator.verify_doc_token(&token, "doc123", 0), Ok(())); assert_eq!(authenticator.verify_doc_token(&token, "doc123", custom_expiration * 1000), Ok(())); assert_eq!( authenticator.verify_doc_token(&token, "doc123", (custom_expiration + 1) * 1000), Err(AuthError::Expired) ); } #[test] fn test_never_expires() { let authenticator = Authenticator::gen_key().unwrap(); let token = authenticator.gen_doc_token("doc123", ExpirationTimeEpochMillis::max()); assert_eq!(authenticator.verify_doc_token(&token, "doc123", u64::MAX), Ok(())); }These additional test cases would help ensure the robustness of the expiration time handling.
Also applies to: 406-406, 441-441, 455-455, 466-466
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- crates/y-sweet-core/src/auth.rs (10 hunks)
- crates/y-sweet-worker/src/lib.rs (3 hunks)
- crates/y-sweet/src/server.rs (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/y-sweet/src/server.rs
🧰 Additional context used
🔇 Additional comments (6)
crates/y-sweet-worker/src/lib.rs (4)
11-14: LGTM: New imports are consistent with functionality changesThe new imports for
AuthDocRequest,ExpirationTimeEpochMillis, andDEFAULT_EXPIRATION_SECONDSalign well with the changes in theauth_docfunction, supporting the new token validity time feature.
213-215: LGTM: Expiration time calculation looks goodThe expiration time calculation correctly uses the
valid_for_secondsfrom the request body or falls back toDEFAULT_EXPIRATION_SECONDS. The use ofExpirationTimeEpochMillistype improves type safety for expiration time handling.
220-220: LGTM: Token generation with configurable expiration timeThe token generation now correctly uses the calculated expiration time, which can be specified in the request or defaults to a predefined value. This change successfully implements the feature of configurable token validity times.
Line range hint
194-220: Summary: Successfully implemented configurable token validity timesThe changes in the
auth_docfunction effectively implement the feature of allowing token validity time to be specified through the API, as outlined in the PR objectives and linked issues. The use ofExpirationTimeEpochMillisenhances type safety, and the fallback toDEFAULT_EXPIRATION_SECONDSensures backward compatibility.However, there's room for improvement in error handling during request parsing. Consider addressing this to provide better feedback to API consumers in case of malformed requests.
Overall, these changes align well with the PR's goals and enhance the flexibility of token management in the system.
crates/y-sweet-core/src/auth.rs (2)
102-102: LGTM! Consistent use of the new type.The change in the
Payloadstruct to useOption<ExpirationTimeEpochMillis>for theexpiration_millisfield is consistent with the introduction of the new type. This maintains the optional nature of the expiration time while improving type safety.
277-283: LGTM! Improved expiration check.The changes to the
verifymethod correctly incorporate the newExpirationTimeEpochMillistype. The use ofunwrap_or(ExpirationTimeEpochMillis::max())is a clean and effective way to handle cases where no expiration time is specified.
| @@ -34,6 +34,19 @@ pub struct AuthDocRequest { | |||
| pub user_id: Option<String>, | |||
| #[serde(default)] | |||
| pub metadata: HashMap<String, Value>, | |||
| #[serde(skip_serializing_if = "Option::is_none", rename = "validForSeconds")] | |||
| pub valid_for_seconds: Option<u64>, | |||
There was a problem hiding this comment.
Even though this value doesn't exactly make sense in a Jamsocket context, I suppose we should at least allow it to be a part of the request body (and probably log if we see it, too)
There was a problem hiding this comment.
If we expose the same parameter in Plane, we could make it work there, too. Plane has an equivalent internally.
There was a problem hiding this comment.
Adds
valid_for_secondsto the auth body request. We weren't actually parsing the request body before, so we do that now.Introduces a newtype to avoid mixing up the current time and expiration times.
Closes #309