Add multi-profile authentication support - #293
Conversation
8e7745e to
24c32c4
Compare
Displays the email and workspace for the active token, with proper error messages for unauthenticated and invalid-token cases.
sergey-borovkov
left a comment
There was a problem hiding this comment.
Review — Multi-profile authentication support
Overview
Replaces the plain-text ~/.screenly single-token file with a YAML profile store, adding login --name, logout --name, auth list, auth switch, and me commands. Plain-text legacy files are transparently migrated. The migration design (auto-upgrade plain text → default profile on first write) is well thought out, and there's solid unit-test coverage for Authentication state transitions.
Correctness
-
Token file permissions are not restricted. The YAML file now holds multiple tokens but is still written via
fs::writewith default umask. The old behavior had the same flaw, but the blast radius is larger now (compromise of one file = all profiles). Considerfs::set_permissions(&path, Permissions::from_mode(0o600))on Unix afterwrite_store(src/authentication.rswrite_store). -
remove_tokenpost-delete active selection is non-deterministic. When removing the active profile,store.active = store.tokens.keys().next().cloned()picks an arbitraryHashMapkey — different on each run. Sort or pick deterministically (e.g., alphabetical first), andinfo!()the new active profile name so the user knows what just happened. -
Logoutpanics on any error via.expect(\"Failed to remove token.\")incli.rs. Pre-existing pattern, but the newremove_tokenhas more failure modes (NoCredentials, ProfileNotFound, Yaml parse). Better to print a friendly message andexit(1), matching other auth paths. -
fetch_profile_infoassumes/v4.1/users/mereturns an array.user.get(0).and_then(|u| u[\"email\"].as_str())only works if the response is[{...}], not{...}. The tests mock both shapes consistently, but please confirm the live API actually returns an array — if it returns an object,mewill silently print\"unknown\". -
No User-Agent header on profile-info requests. Existing
Authentication::client()setsscreenly-cli {version};fetch_profile_infobuilds a bare client. Minor, but inconsistent. -
Race on store updates.
read → mutate → writeinverify_and_store_token/remove_token/switch_profileisn't atomic; two concurrentscreenly logininvocations can lose writes. CLI use makes this unlikely, but a temp-file + rename would be cheap insurance.
Style / Conventions
-
Duplicated table-rendering block in
AuthCommands::ListandAuthCommands::Switch { name: None }— ~25 identical lines. Extract aprint_profiles_table(profiles, api_url)helper. -
AuthCommands::Switchwith nonameprinting the list is undocumented. The clap help reads<NAME> — Profile name to activate, with no hint that omitting it is a meaningful action. Either document it (/// Without an argument, prints the profile list.) or makenamerequired. -
Me"Profile:" line shown conditionally based onactive_profile_name(). WhenAPI_TOKENenv var is in use, the field is silently omitted in both human and JSON output. Reasonable, but consider printingProfile: (from API_TOKEN env)so the user understands why no profile name appears. -
Redundant
use serde_yaml;at the top ofsrc/authentication.rs— theserde_yaml::Errorreference in the enum already pulls in the crate path. -
#[error(\"yaml error\")]swallows the underlying message. Use#[error(\"yaml error: {0}\")]so parse failures are diagnosable.
Test coverage
Good additions for migration, switch, list, and fetch_profile_info. Missing:
loginrequires--namewhen multiple profiles exist — the new safety check is the headline UX claim of the PR and isn't covered.verify_and_store_tokenpreserves existing profiles when called with a newname— easy to regress if someone refactors toTokenStore::default().remove_token(Some(name))with explicit name — only theNone(active) path is tested.- Remove-active-profile picks a remaining one as new active — would also catch the non-determinism above.
Performance
auth listandauth switch(no arg) make 2 sequential HTTP requests per profile (users/me+teams). For 5+ profiles this is a noticeable hang. Consider parallelizing withfutures::join_all(already a dep), or calling a combined endpoint if available.
Security
- See file-permissions point above — primary concern.
- Tokens are passed around in
(name, token, is_active)tuples returned fromlist_profiles(). Internal-only, but token strings are unused in the caller — consider returning(name, is_active)and exposing token lookup via a separate method to reduce accidental-print risk.
Summary
Solid feature with a clean migration path. Top priorities before merge: deterministic active-profile selection on removal, file permissions (0o600), graceful logout error handling, and tests for the --name enforcement and explicit-name removal. The duplicate table code and undocumented switch no-arg behavior are easy follow-ups.
Drop the redundant serde_yaml import (the crate path is already
pulled in by the AuthenticationError enum) and surface the underlying
parse error via {0} so YAML failures are diagnosable.
remove_token now picks the alphabetically first remaining profile as the new active one instead of an arbitrary HashMap key, and returns the resulting active profile name. The Logout handler reports that name (or that no profiles remain) and prints a friendly message with exit(1) for NoCredentials, ProfileNotFound, and other errors instead of panicking.
write_store now writes to a temp file and renames it over the target so a crash or concurrent read can't observe a truncated store. On Unix the file is chmod'd to 0o600 since it holds every profile's token.
Extract authenticated_client() so fetch_profile_info sends the same screenly-cli User-Agent as the rest of the CLI instead of a bare client. Parse /v4.1/users/me whether it returns a single object or a one-element array, so 'me' no longer silently prints 'unknown' if the API returns an object.
list_profiles now returns (name, is_active) pairs; profile details are fetched via the new fetch_profiles_with_info, which keeps tokens inside the authentication module. The duplicated ~25-line table-rendering block in auth list and auth switch (no arg) is replaced by a single print_profiles_table helper.
auth list and auth switch (no arg) previously issued their per-profile API requests one profile at a time, so latency grew linearly with the number of profiles. Because the client is reqwest::blocking, use rayon (already a dependency) rather than futures::join_all to run them across threads.
Document that 'auth switch' without an argument prints the profile list, in both the subcommand and the argument help. When the token comes from the API_TOKEN env var, 'me' now shows 'Profile: (from API_TOKEN env)' in both human and JSON output instead of silently omitting the field.
Move the login profile-name resolution out of handle_cli into a pure resolve_login_name helper so the '--name required when a profile already exists' rule can be unit-tested. Behavior is unchanged.
Cover the gaps called out in review: - resolve_login_name: fresh-install default, explicit name, and the --name-required rule when a profile already exists. - verify_and_store_token preserves existing profiles when storing a new one. - remove_token by explicit name (non-active), unknown name error, and deterministic new-active selection when the active profile is removed. - fetch_profile_info accepts an object response, not just an array. - write_store restricts the store to 0o600 on Unix.
…-auth # Conflicts: # src/cli.rs
|
Thanks @sergey-borovkov, good catches. Pushed a bunch of changes (and merged master in). Quick rundown: Permissions and the write race are both handled now. Fixed the non-deterministic active selection on removal. It sorts the remaining profiles and takes the first one now, and On the users/me thing, I went and checked against prod with a real token. It does return an array, Rest of the smaller stuff: pulled the User-Agent into a shared client builder so One thing I did differently from your suggestion: the client is blocking, so Added tests for the |
sergey-borovkov
left a comment
There was a problem hiding this comment.
Review
Nice feature, and the storage layer is a real improvement over what it replaces: atomic tmp+rename write, 0600 permissions (the old file was 0644), the trailing newline is now trimmed, and logout on a missing file no longer panics through .expect().
I checked out the head commit to verify: cargo clippy --all-targets -- -D warnings is clean and all 18 authentication:: tests pass. Everything below is from reading the code plus running the built binary against a fake $HOME.
Blocking
screenly loginnow fails for every existing user. After migration everyone has exactly one profile, so plainscreenly login— the normal flow after a token rotation — exits 1 with "A profile already exists". Details inline.- A malformed store is silently reinterpreted as a raw token, and the profiles are destroyed on the next write. Reproduced with a one-character typo. Details inline.
Should fix
meignores the global--outputand reintroduces a second--jsonflag.mereports the wrong profile name whenAPI_TOKENis set.- The profile table misaligns whenever a name or email is shorter than its header, or when info is missing.
auth switchwith no argument swallows errors and exits 0.
Minor
- Permission window on the tmp file, and a fixed tmp filename that two concurrent
logins can race on (both inline). NoCredentialshas no friendly message:logoutno longer deletes the file, so the logged-out state now producesNoCredentialsrather thanIo(NotFound), and the nice "Not logged in. Please runscreenly login" branch is effectively dead for that path. Worth adding aNoCredentialsarm.- No per-profile API URL (inline) — worth deciding now while the format is new.
list_profilesreturningVec<(String, bool)>reads poorly at call sites; a small struct (or reusingProfileEntrywithinfo: None) would be clearer.Commands::Mebuilds aserde_json::Mapby hand;serde_json::json!is used elsewhere in the repo.
Docs & tests
docs/CommandLineHelp.mdis regenerated correctly.README.mdhas no mention of profiles,auth list/auth switch, orme, which is where users will look first.- Store-layer coverage is solid — migration, removal, deterministic re-activation, permissions, and both
users/meresponse shapes. Gaps worth closing:read_storeon malformed YAML (finding 2 — a test would have caught it),fetch_profiles_with_info(mockable with$HOME+MockServerlike the existing tests), andprint_profiles_tablealignment (finding 5 — easy if it returns aStringinstead ofprintln!-ing). - Nothing asserts that a legacy plain-text file is actually rewritten as YAML on first write, despite that being a headline claim in the description.
| let resolved_name = match resolve_login_name(name.as_deref(), &existing) { | ||
| Some(resolved) => resolved, | ||
| None => { | ||
| error!("A profile already exists. Please specify a profile name with --name."); |
There was a problem hiding this comment.
Blocking: this breaks screenly login for every existing user.
Verified against a legacy plain-text ~/.screenly:
$ screenly login
ERROR A profile already exists. Please specify a profile name with --name. (exit 1)
After migration every current user has exactly one profile, so the most common flow — re-logging in after a token rotation — is a hard break, as is any script or doc that says "run screenly login".
The guard protects against a case that isn't really ambiguous: re-logging in should update the profile you are on. Suggest None => active_profile_name().unwrap_or("default"), keeping the saved under profile 'x' message you already emit so the target is explicit.
Related: Authentication::list_profiles().unwrap_or_default() on the line above turns a store read error into "no profiles", which combined with the fallback below silently overwrites.
There was a problem hiding this comment.
Fixed in df627f5. resolve_login_name no longer errors when a profile already exists. Plain screenly login now updates the active profile (via active_profile_name()), falling back to default only on a fresh install, which is exactly the re-login-after-rotation flow you described. I also dropped the list_profiles().unwrap_or_default() bit, so a store read error can't masquerade as "no profiles" and trigger an overwrite. Covered by test_resolve_login_name_defaults_to_active_profile.
| if let Ok(store) = serde_yaml::from_str::<TokenStore>(&contents) { | ||
| return Ok(store); | ||
| } | ||
| // Backward compat: plain text token → migrate to "default" profile |
There was a problem hiding this comment.
Blocking: this fallback fires on any deserialization failure, not just genuine plain-text files — so a malformed store becomes a single bogus token and the real profiles are destroyed on the next write.
Reproduced with a one-character typo (tokenz: instead of tokens:):
$ screenly auth list
Profile Email Workspace
* default <-- the real 'prod' profile is gone
$ screenly logout
$ cat ~/.screenly
active: null
tokens: {} <-- prod_token_secret is now unrecoverable
tokens is a required field (a missing active is fine — serde defaults Option to None), so any hand-edit that touches it, any truncated file, or any future schema tightening silently converts credentials to garbage.
Gate the fallback on something that actually looks like a legacy token — a single non-empty line with no : and no newline — and return AuthenticationError::Yaml otherwise so the user can fix the file. That is also the one place the new Yaml variant would be useful; today it can only surface from write_store.
There was a problem hiding this comment.
Fixed in 6297cf6, with the error message made friendly in a50ee24. read_store now only migrates when the file actually looks like a legacy token (a single non-empty line with no :), gated by is_legacy_token. Any other parse failure returns AuthenticationError::CorruptStore instead of being reinterpreted, so the stored profiles survive. The error names the file path and tells the user to fix it or delete it and run screenly login, and it makes clear the file is left untouched.
I reproduced your tokenz: case against the built binary: it now exits 1 with that message and the file stays intact. Tests: test_read_store_malformed_yaml_returns_error_not_token and test_legacy_plain_text_is_rewritten_as_yaml_on_first_write.
There was a problem hiding this comment.
Confirmed fixed — I rebuilt at a50ee24 and reproduced the tokenz: case: it exits 1 on both auth list and logout, and the file comes back byte-identical. is_legacy_token still migrates a genuine single-line token, and the error text is clear about what to do. Nice work on the rest of the thread too; I verified each of the other fixes against the built binary and they all behave as described.
One small residual gap in this change: a zero-byte ~/.screenly now takes the CorruptStore path rather than being treated as an empty store.
$ : > ~/.screenly && screenly login
ERROR The credentials file at ~/.screenly could not be parsed (missing field `tokens`).
Fix its contents, or delete it ... and run `screenly login` to start fresh. (exit 1)
is_legacy_token("") is false, so an empty file falls through to the error. Two small annoyances: the message says "missing field tokens" for a file that has no fields at all, and it blocks login — the very command the message tells you to run — so the user has to delete the file first.
read_store already returns TokenStore::default() for a file that doesn't exist; a file that exists but is empty or whitespace-only should behave the same. Roughly:
if contents.trim().is_empty() {
return Ok(TokenStore::default());
}Non-blocking from my side — it's recoverable since the error names the file, so fix it or wave it through as you prefer.
There was a problem hiding this comment.
Good catch, fixed in 855bca3. read_store now returns an empty store for a zero-byte or whitespace-only file, the same as a missing one, so it no longer takes the CorruptStore path and no longer blocks login. Verified live: me on an empty file now reports "Not logged in" and login proceeds. Added a test covering both empty and whitespace-only contents.
Thanks for re-verifying the rest against the binary.
| /// Show information about the currently authenticated profile. | ||
| Me { | ||
| /// Enables JSON output. | ||
| #[arg(short, long, action = clap::ArgAction::SetTrue)] |
There was a problem hiding this comment.
master recently standardized on the global -o/--output table|json|csv, with --json deprecated and hidden. A local -j/--json here means screenly me --help shows both flags and --output json is silently ignored:
Usage: screenly me [OPTIONS]
-j, --json Enables JSON output
-o, --output <OUTPUT> Output format: table (default), json, or csv
(clap skips propagating the global json because the subcommand shadows the id, so there is no panic — just two flags with different meanings.)
Suggest dropping the local flag and threading output/OutputType through, implementing Formatter for the profile info the way Screens/Assets do. The same applies to auth list, which is a table command that currently cannot emit JSON or CSV.
There was a problem hiding this comment.
Fixed in df627f5. Dropped the local --json flag. Me {} now threads the global --output through a Formatter impl (ProfileDetails) that supports table, json, and csv. Did the same for auth list via a ProfilesTable formatter, so it can emit json/csv too instead of being table-only.
| match fetch_profile_info(&auth.token, &auth.config.url) { | ||
| Ok(info) => { | ||
| let json_flag = json.unwrap_or(false); | ||
| let profile = match active_profile_name() { |
There was a problem hiding this comment.
This reports the wrong profile when API_TOKEN is set. active_profile_name() reads the store unconditionally, but read_token() returns the env token whenever API_TOKEN is present — so with both set, me prints Profile: a while actually authenticating as the env token, which is exactly backwards from the (from API_TOKEN env) label you fall back to. Check env::var("API_TOKEN") first.
There was a problem hiding this comment.
Fixed in df627f5. The me label now checks env::var("API_TOKEN") first and prints Profile: (from API_TOKEN env) when the env token is in use, matching read_token's precedence. So it no longer names a stored profile while actually authenticating as the env token. Verified live with both a stored profile and API_TOKEN set.
| /// marking the active profile with a `*`. | ||
| fn print_profiles_table(entries: &[ProfileEntry]) { | ||
| let name_w = entries.iter().map(|e| e.name.len()).max().unwrap_or(0); | ||
| let email_w = entries |
There was a problem hiding this comment.
The table misaligns whenever a name or email is shorter than its header, or when info is missing:
Profile Email Workspace
- ---------
* a
b
Two separate bugs:
- The widths ignore header length — use
max(len, "Profile".len())and likewise forEmail. email_wis0when everyinfoisNone, collapsing the column.
Rows with info: None also drop their columns entirely instead of printing a placeholder, and the user cannot tell whether the token is invalid or the network is down. Consider - or (unavailable) in both columns.
If this returned a String instead of println!-ing, it would be straightforward to unit-test.
There was a problem hiding this comment.
Fixed in df627f5. print_profiles_table became format_profiles_table and returns a String, so it is now unit-tested (test_format_profiles_table_aligns_headers_and_placeholders). Both column widths use max(value_len, header_len), and a profile whose info could not be fetched renders (unavailable) in the email and workspace columns instead of dropping them. Verified the alignment live.
| }, | ||
| AuthCommands::Switch { name } => match name { | ||
| None => { | ||
| if let Ok(entries) = fetch_profiles_with_info(&Config::default().url) { |
There was a problem hiding this comment.
if let Ok(entries) = ... prints nothing and exits 0 when the fetch fails. It also does 2N network round-trips just to print a hint — Authentication::list_profiles() gives the same names offline.
Worth deciding deliberately too: switch with no argument silently behaving like list and exiting 0 is unusual for a CLI. A usage error with exit 1, plus the list, is friendlier to scripts.
There was a problem hiding this comment.
Fixed in df627f5. auth switch with no argument is now a usage error: it prints the available profile names and exits 1. The names come from Authentication::list_profiles() (the local store), so it does no network round-trips.
| // Write to a temp file and rename over the target so a concurrent reader | ||
| // never observes a half-written store and a crash mid-write can't corrupt it. | ||
| let tmp_path = path.with_extension("tmp"); | ||
| fs::write(&tmp_path, contents)?; |
There was a problem hiding this comment.
fs::write creates the tmp file at 0644 and restrict_permissions chmods it afterwards, so the token is briefly world-readable. Creating it with OpenOptions::new().mode(0o600) closes the window.
Separately, .screenly.tmp (line above) is a fixed name shared by all processes, so two concurrent logins can interleave writes into it — which partly undercuts the atomicity the comment claims. A pid/random suffix fixes it, or tempfile::NamedTempFile::persist, since tempfile is already a dependency.
There was a problem hiding this comment.
Fixed in 6297cf6. write_store now creates the temp file through create_private_file, which opens it with mode 0o600 from the start, so there is no window where the token is world-readable. The temp filename also carries the process id, so two concurrent logins can't interleave into a shared .screenly.tmp. I didn't reach for tempfile here since it's only a dev-dependency in this crate.
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Default)] | ||
| struct TokenStore { |
There was a problem hiding this comment.
The store holds only tokens, so auth list sends every stored token to the single API_BASE_URL. Given that multi-profile mostly means multi-environment, a stage token gets transmitted to the prod host (and then shows blank because it 401s).
Storing a url per profile is worth considering now, while the format is new — it is much cheaper than a second migration later.
There was a problem hiding this comment.
Good point, and I agree it's worth deciding while the format is new. I've deliberately left it out of this PR rather than rush it: doing it properly changes the store schema, the migration, read_token/Authentication::new, and fetch_profiles_with_info, and it hinges on a decision I'd rather make explicitly than guess at, namely how a profile's URL gets captured (a login --url flag, capturing API_BASE_URL at login time, or something else). I'd like to do it as its own follow-up PR with that decision settled. I can open a tracking issue if you want.
There was a problem hiding this comment.
Opened #303 to track this as a follow-up, including the open decision on how a profile's URL should be captured (--url flag vs. capturing the resolved base URL at login time). Keeping it out of this PR so we can settle that separately.
| AuthenticationError::Io(io_err) if io_err.kind() == std::io::ErrorKind::NotFound => { | ||
| "Not logged in. Please run `screenly login` first to authenticate.".to_string() | ||
| } | ||
| AuthenticationError::ProfileNotFound(name) => { |
There was a problem hiding this comment.
Since remove_token no longer deletes the file, the ordinary logged-out state now yields NoCredentials rather than Io(NotFound), so the friendly "Not logged in. Please run screenly login first" branch above is effectively dead for that path and users get the generic Authentication error: no credentials error instead. Worth adding a NoCredentials arm here.
There was a problem hiding this comment.
Fixed in df627f5. get_authentication_error_message now has a NoCredentials arm that returns the "Not logged in. Please run screenly login first to authenticate." message. Since logout leaves an empty store rather than deleting the file, the logged-out state surfaces as NoCredentials, and it now gets that friendly text instead of the generic error.
Addresses review findings on the storage layer: - Blocking: read_store no longer reinterprets an unparseable store as a plain-text token. A malformed or truncated file (e.g. a mistyped key) now returns AuthenticationError::Yaml instead of collapsing every profile into a single bogus token that the next write would persist. Migration is gated on the file actually looking like a legacy token. - Create the temp file at 0600 from the start (no world-readable window) and give it a per-process name so concurrent writers can't interleave into a shared .screenly.tmp. - list_profiles returns a ProfileSummary struct instead of an opaque (String, bool) tuple. Tests: malformed YAML surfaces an error, a legacy plain-text file is rewritten as YAML on first write, and fetch_profiles_with_info returns per-profile details.
Addresses review findings on the command layer: - Blocking: plain 'screenly login' no longer errors for existing users. It now updates the active profile (falling back to 'default' on a fresh install), so the re-login-after-rotation flow works again. - 'me' drops its local --json flag and honors the global --output (table/json/csv) like other commands, via a Formatter impl. - 'me' derives its profile label with the same API_TOKEN-first precedence as read_token, so it no longer names the stored profile while authenticating as the env token. - 'auth list' honors --output too (table/json/csv). - The profile table accounts for header widths and shows an '(unavailable)' placeholder for unreachable profiles instead of misaligning or dropping columns; it returns a String and is unit-tested. - 'auth switch' with no argument is now a usage error (exit 1) that lists profile names offline instead of silently exiting 0 after 2N network calls. - NoCredentials gets the friendly 'Not logged in' message, which the logged-out path now produces since logout leaves an empty store.
Add a README section covering named profiles, login/logout --name, auth list, auth switch, me, the API_TOKEN override, and legacy-file migration. Regenerate CommandLineHelp.md for the me/login/switch help changes.
Instead of surfacing a raw serde 'missing field' error, a store that fails to parse now returns AuthenticationError::CorruptStore, which names the file's path and tells the user to fix it or delete it and run `screenly login`, and reassures that stored profiles are left untouched. No interactive reset: a corrupt file often still holds recoverable tokens, so the user decides whether to repair or discard it. Documented the behavior in the README.
A zero-byte or whitespace-only credentials file previously took the
CorruptStore path ("missing field tokens") and blocked `login` -- the
very command the error suggests. read_store now returns an empty store
for blank contents, matching the missing-file behavior. Test covers
both empty and whitespace-only files.
|
@sergey-borovkov thanks for the thorough re-review. Everything from this round is in:
All the earlier fixes (login default, malformed-store guard, Ready for another look when you have a moment. |
Summary
Replaces the single-token
~/.screenlyfile with a YAML-based profile store,enabling multiple named authentication profiles with a single active profile at a time.
screenly login --name <profile>stores a token under a named profile and switches to it.Defaults to
"default"on a fresh install. Requires--namewhen other profiles alreadyexist to prevent accidental overwrites.
screenly logout --name <profile>removes a specific profile. Omitting--nameremovesthe active profile.
screenly auth listshows all stored profiles in a table with email and workspace namefetched from the API, with
*marking the active profile.screenly auth switch <name>changes the active profile. Called without an argument,it prints the profile list as a hint.
screenly meshows the email and workspace for the currently active token. Accepts--json. Returns a clear error if not logged in or if the token is invalid.API_TOKENenvironment variable still overrides everything.~/.screenlyfiles (original format) are transparently migrated on first write.