-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(server): cerbos new REEARTH_ACCOUNTS_DEV [FLOW-BE-52] #42
Conversation
WalkthroughThe pull request introduces a new environment variable Changes
Sequence Diagram(s)sequenceDiagram
participant S as Server Start
participant E as Environment
participant C as Cerbos Client
S->>E: Read REEARTH_ACCOUNTS_DEV value
alt Value equals "true"
S->>C: Initialize client with plaintext option
else Value not "true"
S->>C: Initialize client without plaintext option
end
Suggested Reviewers
Poem
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/internal/app/server.go (2)
44-47
: Consider adding a log message when using plaintext modeWhen running with the plaintext option, it would be helpful to add a log message indicating that the insecure connection mode is being used. This provides additional visibility and helps prevent accidental use in production.
if os.Getenv("REEARTH_ACCOUNTS_DEV") == "true" { + log.Info("Using plaintext (insecure) connection for Cerbos as REEARTH_ACCOUNTS_DEV=true") opts = append(opts, cerbos.WithPlaintext()) }
46-46
: Consider a more robust environment variable checkThe current check for the environment variable uses a direct string comparison. A more robust approach would be to use a dedicated function for parsing boolean environment variables that handles different formats ("true", "True", "1", etc.).
- if os.Getenv("REEARTH_ACCOUNTS_DEV") == "true" { + // ParseBool handles "1", "t", "T", "TRUE", "true", "True", + // "0", "f", "F", "FALSE", "false", "False" + if devMode, _ := strconv.ParseBool(os.Getenv("REEARTH_ACCOUNTS_DEV")); devMode {Don't forget to add the import:
import ( // ...existing imports + "strconv" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/.env.example
(1 hunks)server/internal/app/server.go
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/.env.example
🔇 Additional comments (2)
server/internal/app/server.go (2)
44-50
: Improve security by making plaintext connection conditional based on environment - Good change!This change enhances security by making the plaintext connection option for Cerbos conditional rather than hardcoded. Now the plaintext option will only be used in development environments where
REEARTH_ACCOUNTS_DEV=true
, while production environments can use secure connections by default.
44-47
:✅ Verification successful
Verify the environment variable usage across the codebase
Let's verify that the
REEARTH_ACCOUNTS_DEV
environment variable is consistently used and documented throughout the codebase.
🏁 Script executed:
#!/bin/bash # Search for other occurrences of REEARTH_ACCOUNTS_DEV echo "Searching for REEARTH_ACCOUNTS_DEV usage in the codebase..." rg "REEARTH_ACCOUNTS_DEV" --type go --type-add env:.env # Check if the variable is documented in README or other documentation echo "Checking for documentation of REEARTH_ACCOUNTS_DEV..." rg "REEARTH_ACCOUNTS_DEV" --type md rg "REEARTH_ACCOUNTS_DEV" -g "*.env*" # Check the .env.example file echo "Content of .env.example for REEARTH_ACCOUNTS_DEV:" if [ -f ".env.example" ]; then grep "REEARTH_ACCOUNTS_DEV" .env.example elif [ -f "server/.env.example" ]; then grep "REEARTH_ACCOUNTS_DEV" server/.env.example fiLength of output: 745
Environment Variable Usage Verified: Consistent Across the Codebase
The verification confirms that:
- The
REEARTH_ACCOUNTS_DEV
variable is used only inserver/internal/app/server.go
,- Its default value (
false
) is set inserver/.env.example
,- No other occurrences were found in markdown documentation.
No additional modifications are needed.
Overview
What I've done
ref: https://www.notion.so/eukarya/Testing-check-permission-after-migration-1ab16e0fb16580e08d50d15c41362f78?pvs=4#1ab16e0fb1658041a689cc03fe0e3c8a
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit