feat: improvements to "encryption" enhancement #1927
Conversation
- Rename enhancement from "encrypted" to "encryption" for better consistency - Use standard logger for warnings - Throw PrismaClientUnknownError for errors - Disallow using encrypted fields in access policies - More tests
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive update to the encryption-related functionality across multiple files in the project. The primary change involves renaming the enhancement kind from 'encrypted' to 'encryption', which requires modifications to type definitions, enhancement creation, and encryption handling. The changes also include improved error handling, logging mechanisms, and expanded test coverage for encryption scenarios. The updates ensure consistent terminology and enhance the robustness of encryption-related features. Changes
Sequence DiagramsequenceDiagram
participant Client
participant EnhancementCreator
participant EncryptionHandler
participant Validator
Client->>EnhancementCreator: Request enhancement with 'encryption'
EnhancementCreator->>EncryptionHandler: Initialize encryption
EncryptionHandler-->>Validator: Validate encryption constraints
Validator-->>EnhancementCreator: Validation result
EnhancementCreator-->>Client: Enhanced model with encryption
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Nitpick comments (1)
packages/schema/src/language-server/validator/attribute-application-validator.ts (1)
215-221: Implementation detail: scope of the AST traversal.
The new private methodrejectEncryptedFieldscallsstreamAllContents(attr), which traverses the entire attribute node subtree. For simpler policy definitions, this overhead should be minimal. However, if attribute expressions become more complex in the future, consider a more targeted search to avoid potential performance implications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/runtime/src/enhancements/node/create-enhancement.ts(2 hunks)packages/runtime/src/enhancements/node/encrypted.ts(4 hunks)packages/runtime/src/types.ts(1 hunks)packages/schema/src/language-server/validator/attribute-application-validator.ts(4 hunks)tests/integration/tests/enhancements/with-encrypted/with-encrypted.test.ts(5 hunks)
🔇 Additional comments (22)
packages/runtime/src/types.ts (1)
154-154: Renaming 'encrypted' to 'encryption' aligns with the updated enhancement naming.
This change helps keep the naming consistent across the codebase, reducing confusion. No further actions needed.
packages/runtime/src/enhancements/node/create-enhancement.ts (2)
24-24: Renamed 'encrypted' to 'encryption' in ALL_ENHANCEMENTS.
Using 'encryption' is clearer and more consistent with the new terminology. This also helps ensure that the correct enhancement is recognized in the code.
132-132: Updated condition to 'encryption'.
The code now checks kinds.includes('encryption') which matches the rename. Confirm that downstream references and related documentation also reflect this change.
packages/runtime/src/enhancements/node/encrypted.ts (7)
12-14: Importing encryption-related types and logger.
These imports are valid and support the newly introduced logger and encryption types. No issues identified.
31-31: makeProxy target changed to 'encryption'.
Replacing 'encrypted' with 'encryption' ensures alignment with the revamped naming. This change is coherent with other updates.
39-45: New logger property and initialization.
Storing a logger instance in the handler promotes consistent and centralized logging. This implementation is straightforward.
47-47: Throw unknownError if encryption options are missing.
This explicit check provides clear and consistent error messages instead of raw exceptions, improving the developer experience.
51-56: Validations for custom encryption and encryption key.
Providing distinct error messages for missing encryption methods and incorrect key sizes makes error handling granular. The approach is solid.
155-155: Logging decryption failures.
Using this.logger.warn instead of console.warn creates a unified logging approach. Recovered behavior (keeping original value) is acceptable.
172-172: Throw unknownError on encryption failure.
Raising a standardized error ensures consistent error handling across enhancements. This is a good practice.
tests/integration/tests/enhancements/with-encrypted/with-encrypted.test.ts (9)
2-2: New import loadModelWithError.
Adding this import for error-based tests is clear and helpful. It keeps test code concise and focused.
7-7: Defining a base64-decoded encryption key.
Defining the key in a test-friendly manner is fine. Use caution if you switch to environment-based secrets in production code.
18-19: Enhancing schema with 'encryption' and verifying stored values.
enhancements: ['encryption']uses the renamed enhancement kind.encryptionKeyis passed and tested correctly.- Checking that the raw data is not the same as the stored cipher text ensures encryption is working as intended.
Also applies to: 25-25, 26-32, 36-36, 57-58, 62-63
64-64: Multi-field encryption test.
This block confirms multiple fields can be encrypted simultaneously.
Result: The test coverage is excellent for verifying consistent encryption on multiple attributes.
Also applies to: 65-101
114-114: Specifying 'encryption' kind for custom encryption test.
Providing kinds: ['encryption'] clarifies which enhancements are active. This is a good approach for specialized testing.
155-165: Validation for non-string fields with @encrypted.
Checking that @encrypted is rejected on non-string types ensures operators do not inadvertently apply encryption to incompatible fields.
167-198: Decryption failure test.
Verifying that the cipher text is returned when decryption fails is an important fallback measure, preventing data loss or crash.
200-229: Length validation on encrypted fields.
Ensuring that the encryption overlay does not break existing validations is crucial. This test demonstrates good coverage.
231-253: Rejecting encrypted fields in policy rules.
Confirming that usage of encrypted fields in policy expressions triggers an error helps maintain robust security boundaries.
packages/schema/src/language-server/validator/attribute-application-validator.ts (3)
28-28: Dependency import usage is consistent.
You've introduced streamAllContents from langium, which aligns with the usage in rejectEncryptedFields(). This looks correct, as you likely need to traverse the AST thoroughly.
142-143: Appropriate enforcement for policy rules.
Calling this.rejectEncryptedFields(attr, accept); ensures that fields annotated with @encrypted are not permitted in model-level rules. This effectively guards against the unintended usage of protected fields in access policies. Good approach!
172-174: Field-level policy checks are strengthened.
Repeating the encrypted-field check in field-level policy rules extends the same protection. This duplication is necessary as the logic for field-level rules differs from model-level rules. The approach is consistent.
Hi @genu , FYI I'm making a couple of minor improvements to the encryption feature.