Skip to content

Add check to only allow users on the enterprise plan to run tier 1#721

Merged
jdalton merged 1 commit intomainfrom
tier1-require-enterprise-plan
Aug 26, 2025
Merged

Add check to only allow users on the enterprise plan to run tier 1#721
jdalton merged 1 commit intomainfrom
tier1-require-enterprise-plan

Conversation

@mtorp
Copy link
Contributor

@mtorp mtorp commented Aug 26, 2025

You can easily modify the code to bypass the check, but you won't be able to see the tier 1 results in the UI or through the API without an enterprise plan, so the check is more informational than actually restrictive.

image

@mtorp mtorp requested a review from jdalton August 26, 2025 13:55
@claude
Copy link

claude bot commented Aug 26, 2025

Claude finished @mtorp's task —— View job


Code Review Complete ✅

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze the changed file for implementation details
  • Review code quality and best practices
  • Check for potential bugs or security issues
  • Evaluate performance considerations
  • Assess architecture and design decisions
  • Provide comprehensive feedback with specific suggestions

Review Feedback

Strengths

  1. Consistent pattern: The implementation follows the existing pattern used in src/commands/login/attempt-login.mts:68 for enterprise plan validation
  2. Clear error messaging: Provides informative error messages with actionable guidance (upgrade link)
  3. Early validation: Checks permissions before expensive operations like Coana execution
  4. Proper error handling: Returns appropriate CResult error structure with detailed cause

⚠️ Areas for Improvement

1. Potential Race Condition / Consistency Issue (src/commands/scan/perform-reachability-analysis.mts:63-64)

const organizations = Object.values(orgsCResult.data.organizations)
const hasEnterprisePlan = organizations.some(org => org.plan === 'enterprise')

Issue: The check uses some() which returns true if ANY organization has an enterprise plan, but doesn't ensure the user is actually using that organization for this scan. A user could have access to multiple orgs (some enterprise, some not) but be running the scan in a non-enterprise context.

Suggestion: Consider validating against the specific organization being used for the scan (if orgSlug is available) or make the logic more explicit about which organization's plan applies.

2. Error Message Clarity (src/commands/scan/perform-reachability-analysis.mts:69-72)

message: 'Tier 1 Reachability analysis requires an enterprise plan',
cause: 'This feature is only available for organizations with an enterprise plan. Please visit https://socket.dev/pricing to upgrade your plan.',

Issue: The message could be more specific about which organization lacks the enterprise plan, especially in multi-org scenarios.

Suggestion: Include organization context in the error message when possible.

3. Performance Consideration (src/commands/scan/perform-reachability-analysis.mts:53)
Issue: The organization fetch happens on every reachability analysis call, adding network latency.

Suggestion: Consider caching organization data with appropriate TTL, or integrate this check into the SDK setup phase to avoid repeated API calls.

🔍 Security Assessment

  • Low Risk: The client-side check is appropriately described as "informational" since server-side enforcement occurs in the UI/API
  • Access Control: Proper use of organization membership validation
  • No sensitive data exposure: Error messages don't leak internal information

🏗️ Architecture Assessment

  • Separation of concerns: Good use of existing fetchOrganization() function
  • Consistent error patterns: Follows established CResult pattern
  • Early exit strategy: Validates before expensive operations

📝 Minor Suggestions

  1. Consider extracting the enterprise plan validation into a reusable utility function since it's used in multiple places
  2. The comment on line 52 could be more specific: "Check if user has access to enterprise plan for tier 1 reachability analysis"

Overall Assessment

This is a solid implementation that follows existing patterns and provides appropriate user feedback. The main concern is ensuring the plan validation logic works correctly in multi-organization scenarios. The implementation correctly serves its stated purpose as an informational check with server-side enforcement.


@jdalton jdalton merged commit 54c6d50 into main Aug 26, 2025
18 checks passed
@jdalton jdalton deleted the tier1-require-enterprise-plan branch August 26, 2025 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants