fix(desktop): seed auth token in worktree setup to avoid re-authentication#1570
Conversation
…ation Copy encrypted auth token from ~/.superset/ to superset-dev-data/ during workspace setup so new worktrees inherit the existing session.
📝 WalkthroughWalkthroughThis change introduces a new authentication token seeding step to the setup process that copies a pre-existing auth token from the user's home directory into the superset development data directory with appropriate directory and file permissions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.superset/lib/setup/steps.sh (1)
193-194:⚠️ Potential issue | 🟡 MinorStale step-number comment:
step_start_electricstill reads "Step 6" butallocate_port_baseis now step 7 after this PR's renumbering.📝 Proposed fix
- # Step 6 allocates SUPERSET_PORT_BASE; Electric must use that reserved port. + # Step 7 allocates SUPERSET_PORT_BASE; Electric must use that reserved port.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.superset/lib/setup/steps.sh around lines 193 - 194, Update the stale step-number in the comment inside the step_start_electric block: change the reference "Step 6" to "Step 7" so it matches the renumbering after allocate_port_base; locate the comment near the step_start_electric function/section that mentions SUPERSET_PORT_BASE and adjust the step number text to "Step 7".
🧹 Nitpick comments (1)
.superset/lib/setup/steps.sh (1)
464-471:mkdir/chmod 700run before the destination-exists check — inconsistent withstep_seed_local_db.In
step_seed_local_db,mkdir -p "$dev_data_dir"only runs after all guard checks pass. Here, the directory is created and its permissions are modified even when the step is about to be skipped. Reordering to match the sibling pattern avoids an unnecessarychmod 700on an already-existing directory.♻️ Proposed reorder
- mkdir -p "$dev_data_dir" - chmod 700 "$dev_data_dir" - if [ -f "$dest_token" ] && [ "$FORCE_OVERWRITE_DATA" != "1" ]; then warn "Auth token already exists at $dest_token — skipping (use -f/--force)" step_skipped "Seed auth token (already exists)" return 0 fi + mkdir -p "$dev_data_dir" + chmod 700 "$dev_data_dir" + if ! install -m 600 "$source_token" "$dest_token"; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.superset/lib/setup/steps.sh around lines 464 - 471, The mkdir/chmod for "$dev_data_dir" are performed before the guard that skips the step, causing unnecessary creation/permission changes; reorder the code in this step to match step_seed_local_db by moving the mkdir -p "$dev_data_dir" and chmod 700 "$dev_data_dir" to after the guard block that checks [ -f "$dest_token" ] && [ "$FORCE_OVERWRITE_DATA" != "1" ], so the directory is only created and permissions set when the step proceeds (use the same placement pattern as in step_seed_local_db).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.superset/lib/setup/steps.sh:
- Around line 473-477: The copy+chmod sequence using "$source_token" and
"$dest_token" leaves a brief permission exposure and lacks chmod error handling;
replace the two-step cp + chmod 600 with a single atomic install call (install
-m 600 "$source_token" "$dest_token") and propagate the existing error handling
(log via error "Failed to copy auth token" and return 1) if install fails,
removing the separate chmod invocation.
---
Outside diff comments:
In @.superset/lib/setup/steps.sh:
- Around line 193-194: Update the stale step-number in the comment inside the
step_start_electric block: change the reference "Step 6" to "Step 7" so it
matches the renumbering after allocate_port_base; locate the comment near the
step_start_electric function/section that mentions SUPERSET_PORT_BASE and adjust
the step number text to "Step 7".
---
Nitpick comments:
In @.superset/lib/setup/steps.sh:
- Around line 464-471: The mkdir/chmod for "$dev_data_dir" are performed before
the guard that skips the step, causing unnecessary creation/permission changes;
reorder the code in this step to match step_seed_local_db by moving the mkdir -p
"$dev_data_dir" and chmod 700 "$dev_data_dir" to after the guard block that
checks [ -f "$dest_token" ] && [ "$FORCE_OVERWRITE_DATA" != "1" ], so the
directory is only created and permissions set when the step proceeds (use the
same placement pattern as in step_seed_local_db).
| if ! cp "$source_token" "$dest_token"; then | ||
| error "Failed to copy auth token" | ||
| return 1 | ||
| fi | ||
| chmod 600 "$dest_token" |
There was a problem hiding this comment.
cp + chmod 600 leaves a brief exposure window and has no chmod error handling.
cp creates $dest_token with umask-derived permissions (typically 644) before chmod 600 tightens them. If chmod fails (e.g., filesystem issue), the encrypted token remains group/world-readable. Use install -m 600 to set permissions atomically in a single call.
🔒 Proposed fix using install -m 600
- if ! cp "$source_token" "$dest_token"; then
- error "Failed to copy auth token"
- return 1
- fi
- chmod 600 "$dest_token"
+ if ! install -m 600 "$source_token" "$dest_token"; then
+ error "Failed to copy auth token"
+ return 1
+ fi📝 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 ! cp "$source_token" "$dest_token"; then | |
| error "Failed to copy auth token" | |
| return 1 | |
| fi | |
| chmod 600 "$dest_token" | |
| if ! install -m 600 "$source_token" "$dest_token"; then | |
| error "Failed to copy auth token" | |
| return 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.superset/lib/setup/steps.sh around lines 473 - 477, The copy+chmod sequence
using "$source_token" and "$dest_token" leaves a brief permission exposure and
lacks chmod error handling; replace the two-step cp + chmod 600 with a single
atomic install call (install -m 600 "$source_token" "$dest_token") and propagate
the existing error handling (log via error "Failed to copy auth token" and
return 1) if install fails, removing the separate chmod invocation.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
SUPERSET_HOME_DIRto a worktree-localsuperset-dev-data/directory, but never copies the auth token there~/.superset/auth-token.encuses the machine's hardware UUID for encryption, so it's portable across worktrees on the same machinestep_seed_auth_tokento the setup script that copies the token, matching the existingstep_seed_local_dbpatternChanges
.superset/lib/setup/steps.sh— Addedstep_seed_auth_token()function that copies~/.superset/auth-token.enctosuperset-dev-data/auth-token.encwith0600permissions.superset/lib/setup/main.sh— Added step 5 (seed auth token) after seeding the local DB, renumbered subsequent stepsTest Plan
-f/--forceflag overwrites an existing tokenSummary by CodeRabbit