Add E2EE under premium flow #166
Conversation
WalkthroughThis pull request introduces several significant changes across the project, focusing on integrating RevenueCat for in-app purchases, modifying space and user models, and updating space management logic. The changes include adding RevenueCat configuration, introducing user types (free and premium), enhancing space encryption handling, and updating build configurations. The modifications span multiple files in the app, data, and configuration layers, introducing new properties, methods, and dependency management strategies. Changes
Sequence DiagramsequenceDiagram
participant App as YourSpaceApplication
participant RevenueCat
participant User
App->>RevenueCat: Configure Purchases
App->>RevenueCat: Set Log Level to DEBUG
User->>App: Interact with App
alt User Type
User->>App: Check User Type
alt Premium User
App->>App: Allow Advanced Features
else Free User
App->>App: Limit Features
end
end
Possibly related PRs
Suggested reviewers
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: 6
🧹 Nitpick comments (10)
data/src/main/java/com/canopas/yourspace/data/service/space/ApiSpaceService.kt (6)
52-60: Consider using a custom exception for unauthorized actions.Throwing
IllegalStateExceptionwhen a user attempts to create an encrypted space without the appropriate permissions may not be the best practice. It is more appropriate to use a custom exception or anAuthenticationExceptionto clearly indicate the nature of the error.Apply this diff to introduce a custom exception:
+class UnauthorizedActionException(message: String) : Exception(message) suspend fun createSpace(spaceName: String, enableEncryption: Boolean = false): String { val spaceId = UUID.randomUUID().toString() val docRef = spaceRef.document(spaceId) val currentUser = authService.currentUser ?: throw IllegalStateException("No authenticated user") if (enableEncryption && currentUser.user_type == FREE_USER) { - // Redirect to subscription page - throw IllegalStateException("User must be a premium user to enable encryption") + throw UnauthorizedActionException("User must be a premium user to enable encryption") }
57-60: Update comment to match implementation or implement redirection logic.The comment
// Redirect to subscription pagesuggests a redirection, but the implementation throws an exception. Either update the comment to reflect that an exception is thrown, or implement the redirection logic if appropriate.Apply this diff to update the comment:
if (enableEncryption && currentUser.user_type == FREE_USER) { - // Redirect to subscription page + // Throw exception because user must be a premium user to enable encryption throw UnauthorizedActionException("User must be a premium user to enable encryption") }
82-118: Simplify the flow control injoinSpacemethod.The
whenblock with multiple conditions and nested logic may be hard to read and maintain. Consider refactoring the method to simplify the logic and improve readability.Here's a possible refactor:
suspend fun joinSpace(spaceId: String, role: Int = SPACE_MEMBER_ROLE_MEMBER, enableEncryption: Boolean? = null) { val user = authService.currentUser ?: throw IllegalStateException("No authenticated user") val isEncryptionEnabled = enableEncryption ?: getSpace(spaceId)?.is_encryption_enabled ?: throw IllegalStateException("Space not found") if (isEncryptionEnabled && user.user_type == FREE_USER) { throw UnauthorizedActionException("User must be a premium user to join an encrypted space") } joinSpace(spaceId, user, role) if (isEncryptionEnabled && user.user_type == PREMIUM_USER) { // Update the "docUpdatedAt" so others see membership changed val docRef = spaceGroupKeysDoc(spaceId) docRef.update("doc_updated_at", System.currentTimeMillis()).await() // Distribute sender key to all members distributeSenderKeyToSpaceMembers(spaceId, user.id) } else if (!isEncryptionEnabled && user.user_type == PREMIUM_USER) { // Notify user about joining an unencrypted space // Implementation of notification logic goes here } }
89-91: Clarify the handling of subscription redirection.As in the
createSpacemethod, the comment suggests redirection to a subscription page, but the code throws an exception. Ensure consistency in how this scenario is handled and consider implementing the redirection logic if applicable.Apply this diff to update the comment:
when { isEncryptionEnabled && user.user_type == FREE_USER -> { - // Redirect to subscription page + // Throw exception because user must be a premium user to join an encrypted space throw UnauthorizedActionException("User must be a premium user to join an encrypted space") }
120-141: Handle exceptions more specifically injoinSpacemethod.Catching a general
Exceptionmay obscure specific issues that could be handled differently. Consider catching specific exceptions or rethrowing them with additional context.Apply this diff to catch specific exceptions:
try { spaceMemberRef(spaceId) .document(user.id).also { val member = ApiSpaceMember( space_id = spaceId, user_id = user.id, role = role, identity_key_public = user.identity_key_public, location_enabled = true ) it.set(member).await() } apiUserService.addSpaceId(user.id, spaceId) -} catch (e: Exception) { +} catch (e: FirebaseFirestoreException) { Timber.e(e, "Failed to join space $spaceId") throw e }
83-86: Improve error message when space is not found.Throwing
IllegalStateException("Space not found")may not provide enough context. Consider using a more specific exception with additional details.Apply this diff to improve the exception:
val isEncryptionEnabled = enableEncryption ?: kotlin.run { getSpace(spaceId)?.is_encryption_enabled ?: throw IllegalArgumentException("Space with ID $spaceId not found") }data/src/main/java/com/canopas/yourspace/data/models/user/ApiUser.kt (1)
13-15: Consider using anenumforuser_typefor type safety.Using an
enum classinstead of constants forFREE_USERandPREMIUM_USERcan enhance type safety and readability.Apply this diff to define an
enum:-const val FREE_USER = 0 -const val PREMIUM_USER = 1 +enum class UserType(val value: Int) { + FREE_USER(0), + PREMIUM_USER(1) +} data class ApiUser( val id: String = UUID.randomUUID().toString(), // ... - val user_type: Int = FREE_USER, + val user_type: UserType = UserType.FREE_USER, // ... )And update usages accordingly.
data/src/main/java/com/canopas/yourspace/data/models/space/ApiSpace.kt (1)
14-14: Document theis_encryption_enabledproperty.Adding documentation to the
is_encryption_enabledproperty will help other developers understand its purpose and default behavior.Apply this diff to add a comment:
data class ApiSpace( val id: String = UUID.randomUUID().toString(), val admin_id: String = "", val name: String = "", /** Indicates whether encryption is enabled for this space. */ val is_encryption_enabled: Boolean = false, val created_at: Long? = System.currentTimeMillis() )data/src/main/java/com/canopas/yourspace/data/repository/SpaceRepository.kt (1)
Line range hint
1-300: Consider these architectural recommendations for E2EE premium integration:
- Implement clear state transitions for encryption status when users upgrade/downgrade between free and premium
- Add migration strategy for existing encrypted spaces
- Consider adding encryption status indicators in the SpaceInfo model
- Document the new E2EE architecture for maintainability
Would you like me to help create a detailed architectural document covering these aspects?
app/build.gradle.kts (1)
234-236: Consider using a version catalog for dependency management.Move the RevenueCat version to a centralized version catalog for better dependency management.
Create a new file
gradle/libs.versions.toml:[versions] revenuecat = "7.0.0" [libraries] revenuecat = { module = "com.revenuecat.purchases:purchases", version.ref = "revenuecat" }Then update the dependency:
- implementation("com.revenuecat.purchases:purchases:7.0.0") + implementation(libs.revenuecat)🧰 Tools
🪛 GitHub Actions: Android Build
[warning] Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/build.gradle.kts(2 hunks)app/src/main/AndroidManifest.xml(1 hunks)app/src/main/java/com/canopas/yourspace/YourSpaceApplication.kt(2 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/pin/setpin/SetPinViewModel.kt(0 hunks)data/build.gradle.kts(1 hunks)data/src/main/java/com/canopas/yourspace/data/models/space/ApiSpace.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/models/user/ApiUser.kt(2 hunks)data/src/main/java/com/canopas/yourspace/data/repository/SpaceRepository.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/service/space/ApiSpaceService.kt(3 hunks)
💤 Files with no reviewable changes (1)
- app/src/main/java/com/canopas/yourspace/ui/flow/pin/setpin/SetPinViewModel.kt
🧰 Additional context used
🪛 GitHub Actions: Android Build
data/build.gradle.kts
[warning] Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0
app/build.gradle.kts
[error] 70-70: Build script references missing local.properties file
[warning] Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0
🔇 Additional comments (8)
data/src/main/java/com/canopas/yourspace/data/service/space/ApiSpaceService.kt (2)
100-115: Ensure notification logic is implemented for unencrypted spaces.The comments indicate that users should be notified when joining unencrypted spaces. However, there is no implementation of this notification. Consider adding the necessary logic to inform premium users about the implications.
Do you want to implement the notification logic to alert premium users when they join unencrypted spaces? If so, I can help draft the necessary code.
110-112: Ensure sender key distribution handles errors gracefully.In
distributeSenderKeyToSpaceMembers, ensure that any exceptions during distribution do not leave the system in an inconsistent state. Consider adding error handling or retries if necessary.Would you like to review the
distributeSenderKeyToSpaceMembersfunction to ensure robust error handling?data/src/main/java/com/canopas/yourspace/data/models/user/ApiUser.kt (1)
30-30: Default values should reflect the intended default state.The
user_typedefaulting toFREE_USERmay be acceptable, but ensure that this aligns with business logic, especially if new user types are added in the future.Please confirm that all instances where
ApiUseris instantiated without specifyinguser_typeshould indeed default toFREE_USER.data/src/main/java/com/canopas/yourspace/data/repository/SpaceRepository.kt (2)
71-72: LGTM! Simplified list emptiness check.The change from filtering null values to directly checking if the list is empty is a cleaner approach.
Line range hint
1-300: Verify the E2EE key management strategy after removal of sender keys generation.The removal of
generateAndDistributeSenderKeysForExistingSpacesmethod suggests a significant change in how E2E encryption keys are managed. Since this PR moves E2EE under the premium flow, we need to ensure:
- The key generation and distribution logic has been properly migrated to the premium flow
- Existing spaces' encryption is handled correctly during the transition
- There's proper fallback for non-premium users
Let's verify the E2EE implementation:
✅ Verification successful
E2EE implementation properly handles premium flow transition
The codebase shows a well-structured E2EE implementation with proper premium-gating:
- Free users are prevented from enabling encryption or joining encrypted spaces
- Premium users get appropriate warnings when joining unencrypted spaces
- Key management infrastructure remains intact with proper validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for new E2EE key management implementation echo "Searching for E2EE related implementations..." # Look for premium-related encryption handling rg -l "premium.*encrypt|encrypt.*premium" --type kotlin # Look for key generation and distribution logic ast-grep --pattern 'fun $FUNC($PARAMS) { $$$ encrypt$$$ $$$ }' # Check for migration handling rg -l "migration.*encrypt|encrypt.*migration" --type kotlinLength of output: 370
Script:
#!/bin/bash # Search for the removed method and E2EE related code echo "Searching for sender keys and E2EE implementations..." # Look for the removed method rg -l "generateAndDistributeSenderKeysForExistingSpaces" --type kotlin # Look for E2EE related code with various terms rg -l "e2e|end.?to.?end|crypto|encrypt|decrypt|keys?" --type kotlin # Check ApiSpaceService.kt content echo "\nChecking ApiSpaceService.kt content:" rg "premium.*encrypt|encrypt.*premium" data/src/main/java/com/canopas/yourspace/data/service/space/ApiSpaceService.kt -A 5 -B 5Length of output: 5237
data/build.gradle.kts (1)
Line range hint
1-100: Address Gradle deprecation warnings.The build is using deprecated Gradle features that will be incompatible with Gradle 9.0.
🧰 Tools
🪛 GitHub Actions: Android Build
[warning] Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0
app/src/main/java/com/canopas/yourspace/YourSpaceApplication.kt (1)
65-65: Verify RevenueCat initialization order.The RevenueCat initialization is placed before user-specific operations but after Places API initialization. This seems appropriate, but please verify that no premium features are accessed before this point.
✅ Verification successful
RevenueCat initialization order is correct ✅
The initialization order is safe as all premium features are properly gated through the server-side API and user type checks. No premium features or RevenueCat purchase checks occur before the initialization in Application.onCreate().
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any RevenueCat purchase checks or premium feature access # that might occur before application onCreate() rg -l "Purchases\." | xargs rg "onCreate|onStart|onResume" -B 5 -A 5Length of output: 953
Script:
#!/bin/bash # Search for RevenueCat purchase checks and premium feature access patterns echo "=== RevenueCat Purchase Checks ===" rg "Purchases\.(shared|cache|check|get|is)" echo -e "\n=== Premium Feature Patterns ===" rg -i "premium|subscription|purchas(e|ed)|entitlement" echo -e "\n=== Early Lifecycle Access ===" ast-grep --pattern 'class $_ : Application { $$$ override fun onCreate() { $$$ } }'Length of output: 2787
app/src/main/AndroidManifest.xml (1)
27-27:⚠️ Potential issueReconsider changing MainActivity launch mode to standard.
Changing from
singleTasktostandardallows multiple instances of MainActivity, which could lead to:
- Inconsistent encryption states across instances
- Premium status verification issues
- Memory leaks from multiple encryption contexts
Consider keeping it as
singleTaskor implementing proper state synchronization across instances.
| suspend fun joinSpace(spaceId: String, role: Int = SPACE_MEMBER_ROLE_MEMBER, enableEncryption: Boolean? = null) { | ||
| val user = authService.currentUser ?: throw IllegalStateException("No authenticated user") | ||
| spaceMemberRef(spaceId) | ||
| .document(user.id).also { | ||
| val member = ApiSpaceMember( | ||
| space_id = spaceId, | ||
| user_id = user.id, | ||
| role = role, | ||
| identity_key_public = user.identity_key_public, | ||
| location_enabled = true | ||
| ) | ||
| it.set(member).await() | ||
| val isEncryptionEnabled = enableEncryption ?: kotlin.run { | ||
| getSpace(spaceId)?.is_encryption_enabled ?: throw IllegalStateException("Space not found") | ||
| } |
There was a problem hiding this comment.
Avoid potential nullability issue with getSpace method.
The getSpace method could return null, leading to a NullPointerException when accessing is_encryption_enabled. Consider using the safe call operator or adding a null check.
Apply this diff to safely handle the null case:
val isEncryptionEnabled = enableEncryption ?: kotlin.run {
- getSpace(spaceId)?.is_encryption_enabled ?: throw IllegalStateException("Space not found")
+ getSpace(spaceId)?.is_encryption_enabled ?: false
}Alternatively, ensure that a proper exception is thrown if the space is not found.
Committable suggestion skipped: line range outside the PR's diff.
data/src/main/java/com/canopas/yourspace/data/models/space/ApiSpace.kt
Outdated
Show resolved
Hide resolved
| @@ -8,7 +8,7 @@ plugins { | |||
|
|
|||
| android { | |||
| namespace = "com.canopas.yourspace.data" | |||
| compileSdk = 34 | |||
| compileSdk = 35 | |||
There was a problem hiding this comment.
Revert compileSdk to 34.
compileSdk 35 is not yet released. The current latest version is 34.
- compileSdk = 35
+ compileSdk = 34📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| compileSdk = 35 | |
| compileSdk = 34 |
🧰 Tools
🪛 GitHub Actions: Android Build
[warning] Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0
app/build.gradle.kts
Outdated
| // RevenueCat | ||
| implementation("com.revenuecat.purchases:purchases:7.0.0") | ||
|
|
There was a problem hiding this comment.
Add ProGuard rules for RevenueCat.
RevenueCat requires specific ProGuard rules to work correctly in release builds.
Add the following to your proguard-rules.pro file:
+# RevenueCat ProGuard Rules
+-keep class com.revenuecat.purchases.** { *; }
+-keep class com.android.billingclient.** { *; }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Android Build
[warning] Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0
| if (System.getenv("REVENUECAT_API_KEY") != null) { | ||
| buildConfigField("String", "REVENUECAT_API_KEY", "\"${System.getenv("REVENUECAT_API_KEY")}\"") | ||
| } else { | ||
| val p = Properties() | ||
| p.load(project.rootProject.file("local.properties").reader()) | ||
| buildConfigField( | ||
| "String", | ||
| "REVENUECAT_API_KEY", | ||
| "\"${p.getProperty("REVENUECAT_API_KEY")}\"" | ||
| ) | ||
| } |
There was a problem hiding this comment.
Improve error handling for missing RevenueCat API key.
The current implementation will crash if REVENUECAT_API_KEY is missing from local.properties.
if (System.getenv("REVENUECAT_API_KEY") != null) {
buildConfigField("String", "REVENUECAT_API_KEY", "\"${System.getenv("REVENUECAT_API_KEY")}\"")
} else {
val p = Properties()
- p.load(project.rootProject.file("local.properties").reader())
- buildConfigField(
- "String",
- "REVENUECAT_API_KEY",
- "\"${p.getProperty("REVENUECAT_API_KEY")}\""
- )
+ val localProperties = project.rootProject.file("local.properties")
+ if (localProperties.exists()) {
+ p.load(localProperties.reader())
+ val apiKey = p.getProperty("REVENUECAT_API_KEY") ?: ""
+ buildConfigField("String", "REVENUECAT_API_KEY", "\"$apiKey\"")
+ } else {
+ buildConfigField("String", "REVENUECAT_API_KEY", "\"\"")
+ logger.warn("local.properties file not found. REVENUECAT_API_KEY will be empty.")
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (System.getenv("REVENUECAT_API_KEY") != null) { | |
| buildConfigField("String", "REVENUECAT_API_KEY", "\"${System.getenv("REVENUECAT_API_KEY")}\"") | |
| } else { | |
| val p = Properties() | |
| p.load(project.rootProject.file("local.properties").reader()) | |
| buildConfigField( | |
| "String", | |
| "REVENUECAT_API_KEY", | |
| "\"${p.getProperty("REVENUECAT_API_KEY")}\"" | |
| ) | |
| } | |
| if (System.getenv("REVENUECAT_API_KEY") != null) { | |
| buildConfigField("String", "REVENUECAT_API_KEY", "\"${System.getenv("REVENUECAT_API_KEY")}\"") | |
| } else { | |
| val p = Properties() | |
| val localProperties = project.rootProject.file("local.properties") | |
| if (localProperties.exists()) { | |
| p.load(localProperties.reader()) | |
| val apiKey = p.getProperty("REVENUECAT_API_KEY") ?: "" | |
| buildConfigField("String", "REVENUECAT_API_KEY", "\"$apiKey\"") | |
| } else { | |
| buildConfigField("String", "REVENUECAT_API_KEY", "\"\"") | |
| logger.warn("local.properties file not found. REVENUECAT_API_KEY will be empty.") | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Android Build
[error] 70-70: Build script references missing local.properties file
[warning] Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0
| Purchases.logLevel = LogLevel.DEBUG | ||
| Purchases.configure(PurchasesConfiguration.Builder(this, BuildConfig.REVENUECAT_API_KEY).build()) |
There was a problem hiding this comment.
Restrict RevenueCat debug logging to debug builds only.
Debug logging is currently enabled for all builds, which could expose sensitive information in production. Consider wrapping this in a debug check.
-Purchases.logLevel = LogLevel.DEBUG
+if (BuildConfig.DEBUG) {
+ Purchases.logLevel = LogLevel.DEBUG
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Purchases.logLevel = LogLevel.DEBUG | |
| Purchases.configure(PurchasesConfiguration.Builder(this, BuildConfig.REVENUECAT_API_KEY).build()) | |
| if (BuildConfig.DEBUG) { | |
| Purchases.logLevel = LogLevel.DEBUG | |
| } | |
| Purchases.configure(PurchasesConfiguration.Builder(this, BuildConfig.REVENUECAT_API_KEY).build()) |
Summary by CodeRabbit
Release Notes
New Features
Changes
Technical Updates