-
Notifications
You must be signed in to change notification settings - Fork 19
Default to unauthenticated client if auth_config.toml not found #198
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
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 (1)
oxen-rust/src/lib/src/api/client.rs (1)
100-100: Consider clarifying the log message.The message "No auth token found for host {}" could be misleading, as it suggests the host is missing from an existing config, when the actual issue is that the config couldn't be loaded at all. The error
eprovides context, but a clearer message would improve debugging.Consider updating the message to better reflect the situation:
- log::debug!("remote::client::new_for_host error getting config: {}. No auth token found for host {}", e, host.as_ref()); + log::debug!("remote::client::new_for_host unable to load auth config: {}. Proceeding without authentication for host {}", e, host.as_ref());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
oxen-rust/src/lib/src/api/client.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
oxen-rust/src/lib/src/api/client.rs (1)
oxen-rust/src/lib/src/config/auth_config.rs (1)
get(59-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Suite / Test Suite (ubuntu-latest)
- GitHub Check: Test Suite / Test Suite (windows-latest)
- GitHub Check: Test Suite / Test Suite (macos-latest)
🔇 Additional comments (1)
oxen-rust/src/lib/src/api/client.rs (1)
96-103: LGTM - implementation correctly handles missing auth config.The graceful fallback to an unauthenticated builder when
AuthConfig::get()fails is appropriate and aligns with the PR objective.AuthConfig::get()returns errors only when the home directory cannot be determined or when the auth config file doesn't exist—not from permission issues, as the function doesn't check file permissions. Both error types warrant proceeding without authentication, making the current error handling correct.The two different error handling strategies at lines 97 (initial setup) and 160 (403 response) are appropriately contextual.
This solves an issue where you couldn't connect to any host, even unauthenticated ones, if auth_config.toml did not exist.
In current main, if auth_config.toml exists but there's no auth token for a host, the client code can still return a client with no authorization. This PR matches that behavior for if auth_config.toml doesn't exist.
To test:
~/.config/oxen/auth_config.tomloxen create-remote --host localhost:3000 --name ox/no-auth --scheme httpto create a new remote. This won't work in current main, but does with this PRYou can also consider connecting to other unauthenticated hosts
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.