fix my account rules #7218
fix my account rules #7218
Conversation
WalkthroughThe pull request modifies entitlement checks within the Changes
Possibly related PRs
Suggested labels
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/main/topLevelPages/myDashboard/Campaigns/CampaignBlock.graphql (1)
12-20: Consider implementing field-level authorizationThe query now exposes sensitive license and entitlement data. Ensure proper authorization checks are implemented on the server side for these fields.
Consider:
- Using GraphQL directives for authorization (@auth, @requiresPermission)
- Implementing field-level resolvers with explicit permission checks
- Documenting the required permissions in the schema
Review query depth and response size
The nested structure (account > license > entitlements) increases the response payload. Consider implementing pagination for the entitlements field if the list can grow large.
Consider adding pagination arguments:
entitlements(first: Int, after: String) { edges { node { ...EntitlementDetails } } pageInfo { hasNextPage endCursor } }src/domain/license/plans/utils/filterAndMapEnabledEntitlements.ts (1)
13-13: Consider combining filter conditions for better performanceWhile the current implementation is correct, we can optimize it by combining the filter conditions to reduce array iterations.
Here's a suggested optimization:
export const filterAndMapEnabledEntitlements = (entitlements: Entitlement[] = []): LicenseEntitlementType[] => { return entitlements - .filter(entitlement => entitlement.enabled) // Filter enabled entitlements - .filter(entitlement => entitlement.isAvailable) // Filter available entitlements + .filter(entitlement => entitlement.enabled && entitlement.isAvailable) .map(entitlement => entitlement.type); // Map to their types };src/main/topLevelPages/myDashboard/Campaigns/CampaignBlock.tsx (2)
15-17: Add error handling for missing license dataThe deeply nested optional chaining could make debugging difficult if the license data is missing or malformed. Consider adding error logging to help diagnose issues in production.
const userEntitlements: LicenseEntitlementType[] | undefined = filterAndMapEnabledEntitlements( - data?.me.user?.account?.license?.entitlements + data?.me.user?.account?.license?.entitlements ?? (() => { + console.warn('License entitlements data is missing or incomplete'); + return undefined; + })() );
20-25: Consider extracting permission logic to a separate utilityThe permission checking logic combines roles and entitlements checks. To improve maintainability and reusability, consider extracting this logic into a separate utility function.
+const AVAILABLE_ROLES = [CredentialType.VcCampaign, CredentialType.BetaTester] as const; +const REQUIRED_ENTITLEMENTS = [LicenseEntitlementType.AccountVirtualContributor] as const; + +const hasRequiredPermissions = ( + roles: CredentialType[] | undefined, + entitlements: LicenseEntitlementType[] | undefined +): boolean => { + return ( + roles?.some(role => AVAILABLE_ROLES.includes(role)) && + entitlements?.some(entitlement => REQUIRED_ENTITLEMENTS.includes(entitlement)) + ) ?? false; +}; -const rolesAvailableTo = [CredentialType.VcCampaign, CredentialType.BetaTester]; -const entitlementsAvailableTo = [LicenseEntitlementType.AccountVirtualContributor]; -if ( - !userRoles?.some(role => rolesAvailableTo.includes(role)) || - !userEntitlements?.some(entitlement => entitlementsAvailableTo.includes(entitlement)) -) { +if (!hasRequiredPermissions(userRoles, userEntitlements)) { return null; }src/domain/community/contributor/Account/ContributorAccountView.tsx (1)
Line range hint
214-223: Add error handling for undefined entitlementsThe canCreate* conditions don't handle cases where entitlements are undefined. This could lead to unexpected behavior.
Consider adding null checks and logging:
const canCreateSpace = - privileges.includes(AuthorizationPrivilege.CreateSpace) && (!isSpaceLimitReached || isPlatformAdmin); + privileges.includes(AuthorizationPrivilege.CreateSpace) && + (spaceFreeEntitlement !== undefined ? (!isSpaceLimitReached || isPlatformAdmin) : false); const canCreateInnovationPack = - privileges.includes(AuthorizationPrivilege.CreateInnovationPack) && - (!isInnovationPackLimitReached || isPlatformAdmin); + privileges.includes(AuthorizationPrivilege.CreateInnovationPack) && + (innovationPackEntitlement !== undefined ? (!isInnovationPackLimitReached || isPlatformAdmin) : false); // Apply similar changes to other canCreate* conditions
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/core/apollo/generated/apollo-hooks.tsis excluded by!**/generated/**src/core/apollo/generated/graphql-schema.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
src/domain/community/contributor/Account/ContributorAccountView.tsx(2 hunks)src/domain/license/plans/utils/filterAndMapEnabledEntitlements.ts(1 hunks)src/main/topLevelPages/myDashboard/Campaigns/CampaignBlock.graphql(1 hunks)src/main/topLevelPages/myDashboard/Campaigns/CampaignBlock.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/domain/community/contributor/Account/ContributorAccountView.tsx (1)
Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
src/domain/license/plans/utils/filterAndMapEnabledEntitlements.ts (1)
Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
src/main/topLevelPages/myDashboard/Campaigns/CampaignBlock.graphql (1)
Pattern src/**/*.{graphql,gql}: Review the GraphQL schema and queries for best practices, potential bugs, and adherence to the project's GraphQL standards.
Ensure that the schema is well-defined and queries are optimized.
Check for security vulnerabilities.
src/main/topLevelPages/myDashboard/Campaigns/CampaignBlock.tsx (1)
Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
🔇 Additional comments (5)
src/main/topLevelPages/myDashboard/Campaigns/CampaignBlock.graphql (1)
17-17: Verify the EntitlementDetails fragment definition
The query references the EntitlementDetails fragment, but its definition is not visible in the provided code.
Let's verify the fragment definition:
✅ Verification successful
EntitlementDetails fragment is properly defined and consistently used
The fragment is well-defined in src/domain/community/user/providers/UserProvider/UserProvider.graphql with all necessary fields including isAvailable, type, limit, usage, etc. It's being consistently used across multiple components that need entitlement information.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for EntitlementDetails fragment definition
# Expected: Find the fragment definition that includes the isAvailable field
# Search for the fragment definition
rg "fragment EntitlementDetails" -A 10
# Search for any references to ensure consistent usage
rg "\.\.\.EntitlementDetails"
Length of output: 2563
src/domain/license/plans/utils/filterAndMapEnabledEntitlements.ts (1)
7-7: Verify the impact of adding required property isAvailable
Adding a required property to the Entitlement type is a breaking change. This could affect any code that constructs Entitlement objects or processes GraphQL responses.
Let's verify all usages of the Entitlement type:
✅ Verification successful
The isAvailable property addition is safe and consistent with the codebase
The addition of isAvailable to the Entitlement type in filterAndMapEnabledEntitlements.ts is actually safe because:
- The
Entitlementtype is locally scoped within this utility file and not exported - The actual GraphQL schema uses
LicenseEntitlementtype, which is different from this local type - The codebase already uses
isAvailableproperty in multiple places:ContributorAccountView.tsxchecksisAvailableon various entitlements- All limit checks (e.g.,
isSpaceFreeLimitReached,isVCLimitReached) rely on this property
The change aligns with existing usage patterns and doesn't introduce any breaking changes since it's modifying a locally scoped type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files that might be affected by the Entitlement type change
# Find potential GraphQL queries using entitlements
echo "Checking GraphQL queries for entitlement fields..."
rg -l "entitlements.*{" --type ts
# Find files that might construct Entitlement objects
echo "Checking for Entitlement object construction..."
rg -l "type.*Entitlement.*=.*{" --type ts
rg -l "interface.*Entitlement.*{" --type ts
Length of output: 659
Script:
#!/bin/bash
# Description: Find all files that might be affected by the Entitlement type change
# Find potential GraphQL queries using entitlements
echo "Checking GraphQL queries for entitlement fields..."
rg "entitlements\s*\{" --type ts
# Find files that might construct Entitlement objects
echo "Checking for Entitlement object construction..."
rg "type\s+Entitlement" --type ts
rg "interface\s+Entitlement" --type ts
# Find all references to Entitlement
echo "Checking all references to Entitlement..."
rg "Entitlement" --type ts
Length of output: 19921
src/main/topLevelPages/myDashboard/Campaigns/CampaignBlock.tsx (2)
2-2: LGTM: Import statements are well-structured
The new imports for license entitlement handling are properly organized and sourced from the correct locations.
Also applies to: 4-4
22-25: LGTM: Robust authorization implementation
The dual-layer authorization check using both roles and entitlements provides strong access control. The implementation effectively prevents unauthorized access to campaign functionality.
src/domain/community/contributor/Account/ContributorAccountView.tsx (1)
188-190:
Verify the security implications of the updated entitlement checks
The new space entitlement checks now require both enabled and !isAvailable conditions. This change could affect access control across the application.
Consider extracting these checks into a dedicated helper function for better maintainability and consistency:
+const isSpaceLimitReached = (entitlement?: LicenseEntitlement) =>
+ entitlement?.enabled && !entitlement?.isAvailable;
-const isSpaceFreeLimitReached = spaceFreeEntitlement?.enabled && !spaceFreeEntitlement?.isAvailable;
-const isSpacePlusLimitReached = spacePlusEntitlement?.enabled && !spacePlusEntitlement?.isAvailable;
-const isSpacePremiumLimitReached = spacePremiumEntitlement?.enabled && !spacePremiumEntitlement?.isAvailable;
+const isSpaceFreeLimitReached = isSpaceLimitReached(spaceFreeEntitlement);
+const isSpacePlusLimitReached = isSpaceLimitReached(spacePlusEntitlement);
+const isSpacePremiumLimitReached = isSpaceLimitReached(spacePremiumEntitlement);
Summary by CodeRabbit
New Features
Bug Fixes
Documentation