feat(user-notification): Firebase Credentials Setup for Local Development #17199
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Logger
User->>CLI: Execute renderSecrets command
CLI->>Logger: Process secret values
Logger-->>CLI: Escape single quotes
CLI->>Logger: Log escaped secret values
Logger-->>User: Display logged values
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
|
rafnarnason
changed the title
Datadog ReportAll test runs ✅ 98 Total Test Services: 0 Failed, 90 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
![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
🧹 Outside diff range and nitpick comments (2)
apps/services/user-notification/README.md (2)
43-44: LGTM! Consider enhancing the note with more details.The warning about cleaning newlines from Firebase credentials is crucial and well-placed. However, it could be more helpful with additional details.
Consider expanding the note with:
> **Important**: For local development, the `FIREBASE_CREDENTIALS` environment variable needs to be cleaned of newlines. After running `get-secrets`, make sure to remove any newline characters from the `FIREBASE_CREDENTIALS` value in your environment. +> +> Example of cleaning newlines: +> ```sh +> # Get the current value +> echo $FIREBASE_CREDENTIALS | tr -d '\n' | pbcopy # On macOS +> # or +> echo $FIREBASE_CREDENTIALS | tr -d '\n' | xclip -selection clipboard # On Linux +> ``` +> +> This step is necessary because newline characters in the credentials can cause Firebase Cloud Messaging authentication failures during local development.
43-44: Consider automating the credentials cleanup.The manual cleanup of newlines is error-prone and could be automated to improve developer experience.
Would you like me to help create a script that:
- Automatically cleans the Firebase credentials after
get-secrets- Validates the credential format before starting the service
- Provides clear error messages if issues are detected
This could be added to the service's development utilities to ensure consistent credential formatting.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/services/user-notification/README.md(1 hunks)apps/services/user-notification/project.json(0 hunks)
💤 Files with no reviewable changes (1)
- apps/services/user-notification/project.json
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/user-notification/README.md (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
…com/island-is/island.is into user-notification/update-readme-env
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/services/user-notification/README.md(1 hunks)apps/services/user-notification/src/config.ts(2 hunks)libs/shared/utils/src/lib/environment.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/services/user-notification/README.md (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/shared/utils/src/lib/environment.ts (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/services/user-notification/src/config.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
…com/island-is/island.is into user-notification/update-readme-env
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
scripts/get-secrets.sh (2)
41-41: Add error handling for ts-node commandConsider adding error handling for the ts-node command execution to provide better feedback if the secrets retrieval fails.
- output=$(ts-node --dir "$ROOT"/infra "$ROOT"/infra/src/cli/cli render-secrets --service="$*") + if ! output=$(ts-node --dir "$ROOT"/infra "$ROOT"/infra/src/cli/cli render-secrets --service="$*"); then + echo "Failed to retrieve secrets for '$*'" >&2 + return 1 + fi
44-48: Consider adding validation for cleaned outputWhile the cleaning process effectively handles newlines, it would be beneficial to validate that the cleaned output maintains valid JSON structure.
echo "$output" | while IFS= read -r line; do cleaned_line=$(echo "$line" | tr -d '\n' | sed 's/\\n/ /g' | sed 's/\\//g') + # Validate JSON structure if line contains JSON + if [[ "$cleaned_line" == *"{"* ]]; then + if ! echo "$cleaned_line" | jq -e . >/dev/null 2>&1; then + echo "Warning: Line contains invalid JSON after cleaning" >&2 + fi + fi echo "$cleaned_line" >> "$env_secret_file" done
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
scripts/get-secrets.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
scripts/get-secrets.sh (1)
Pattern scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
🔇 Additional comments (2)
scripts/get-secrets.sh (2)
37-37: Improved line counting robustness
The use of awk '{print $1}' to extract just the number from wc -l output is more robust than direct usage, as it handles different output formats across systems.
Also applies to: 50-50
40-48: Well-implemented solution for newline handling
The new implementation effectively addresses the Firebase credentials newline issue by:
- Capturing the output separately
- Processing each line individually
- Cleaning newlines and backslashes while preserving JSON format
This solves the authentication failures with Firebase Cloud Messaging mentioned in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
scripts/get-secrets.sh (3)
24-25: Standardize line counting approach across the scriptThe script uses different methods for counting lines (
wc -l | awkhere vswc -l <elsewhere). Consider extracting this into a helper function for consistency and maintainability.+function count_lines() { + wc -l < "$1" +} + function get-secrets { echo "Fetching secret environment variables for '$*'" - pre=$(wc -l "$env_secret_file" | awk '{print $1}') + pre=$(count_lines "$env_secret_file") debug "Project '$*' has $pre secrets before render-secrets"
27-35: LGTM! Clean implementation of credential formattingThe implementation effectively addresses the Firebase credentials formatting issue by properly handling newlines and backslashes while maintaining JSON integrity.
Consider combining the sed commands for better efficiency:
- cleaned_line=$(echo "$line" | tr -d '\n' | sed 's/\\n/ /g' | sed 's/\\//g') + cleaned_line=$(echo "$line" | tr -d '\n' | sed 's/\\n/ /g; s/\\//g')
Line range hint
37-42: Enhance error handling and debugging informationWhile the validation logic is correct, consider providing more detailed feedback when no secrets are found to help developers troubleshoot issues.
post=$(wc -l "$env_secret_file" | awk '{print $1}') - debug "Project '$*' has $post secrets after render-secrets" + debug "Project '$*' has $post secrets after render-secrets (added $((post - pre)) secrets)" if [ "$pre" == "$post" ]; then - echo "No secrets found for project '$*'" + echo "Warning: No secrets found for project '$*'. Please verify:" + echo " 1. The project name is correct" + echo " 2. You have the necessary AWS permissions" + echo " 3. Secrets are configured in AWS Secrets Manager" fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
scripts/get-secrets.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
scripts/get-secrets.sh (1)
Pattern scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
🔇 Additional comments (1)
scripts/get-secrets.sh (1)
20-20: Verify AWS permissions for Firebase credentials
Since this script fetches Firebase credentials from AWS Secrets Manager, ensure that the necessary AWS IAM permissions are documented.
✅ Verification successful
Let me try to gather more information about AWS and Firebase configurations in the codebase.
Let me check the actual script content to verify the AWS permissions usage.
Let me check the related infrastructure code to understand how the secrets are actually fetched.
Based on the code analysis, I can now provide a final response about the AWS permissions verification:
AWS permissions are handled through SSM Parameter Store
The script uses AWS Systems Manager (SSM) Parameter Store to fetch secrets, requiring the following minimal IAM permissions:
ssm:GetParameter- To fetch secretssts:GetCallerIdentity- For AWS authentication check
The script already includes proper AWS authentication verification through the aws-check function before attempting to access any secrets.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if AWS permissions documentation exists
rg -l "aws.*permission|iam.*role" docs/ README.md
# Check for existing Firebase-related secrets
aws secretsmanager list-secrets --query "SecretList[?contains(Name, 'firebase')].Name" --output text 2>/dev/null || echo "No matching secrets found or insufficient permissions"
Length of output: 323
Script:
#!/bin/bash
# Search for AWS and Firebase related configuration files
fd -t f "aws|firebase|secret|credential" -X ls -l
# Look for AWS and Firebase related patterns in the codebase
rg -i "aws|firebase|secret|credential" --type-add 'config:*.{json,yaml,yml,env}' -t config
# Check if there are any IAM or permission related patterns in any file
rg -i "iam|permission|role|policy" -A 3
Length of output: 74582
Script:
#!/bin/bash
# Check the content of get-secrets.sh
cat scripts/get-secrets.sh
# Look for AWS IAM/permission related patterns in the script
rg -p "aws.*iam|assume.*role|get.*secret" scripts/get-secrets.sh
Length of output: 2449
Script:
#!/bin/bash
# Check the render-secrets implementation
cat infra/src/cli/render-secrets.ts
# Check the AWS secrets handling code
cat infra/src/secrets.ts
# Look for AWS IAM roles in infrastructure
rg -p "aws_iam_role|assume_role" infra/
Length of output: 5695
…com/island-is/island.is into user-notification/update-readme-env
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
infra/src/cli/render-secrets.ts (1)
70-73: Consider security and architectural improvementsSeveral architectural improvements could enhance security and maintainability:
- Remove debug logging of sensitive values (line 70)
- Add input validation for secret values
- Consider using a type-safe configuration system
- Add unit tests to verify escaping behavior
Would you like me to help implement any of these improvements? I can:
- Create a type-safe configuration interface
- Add input validation utilities
- Generate unit tests for the escaping logic
rafnarnason
added
automerge
Fixes these types of strings for local development env.secret file so the app runs locally without manual edits to the env value
Before:
export FIREBASE_CREDENTIALS='{\n "type": "service_account",\n "project_id":
After:
export FIREBASE_CREDENTIALS='{
...
"type": "service_account",
"project_id": "mobile-app",
...
}'
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit