Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 124 additions & 4 deletions lore-client/src/cli/commands/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ use crate::println;
use crate::styling::CommonStyles;
use crate::util;

const LOGIN_REMOTE_REQUIRED: &str = "No remote URL supplied. Run `lore auth login <remote-url>` or run from a Lore repository with a configured remote.";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Include the token-login --auth-url remediation in this error path.

Lines 149-150 intentionally allow token login without a remote when explicit --auth-url is present, but Line 41 only tells users to provide a remote URL or repo config. For --token-type/--token users missing --auth-url, the error omits a valid fix.

Suggested adjustment
 const LOGIN_REMOTE_REQUIRED: &str = "No remote URL supplied. Run `lore auth login <remote-url>` or run from a Lore repository with a configured remote.";
+const LOGIN_TOKEN_REMOTE_OR_AUTH_URL_REQUIRED: &str =
+    "No remote URL or auth URL supplied. Run `lore auth login <remote-url>` or pass `--auth-url` for token login outside a Lore repository.";
@@
-    Err(LOGIN_REMOTE_REQUIRED)
+    if args.token_type.is_some() && args.token.is_some() {
+        Err(LOGIN_TOKEN_REMOTE_OR_AUTH_URL_REQUIRED)
+    } else {
+        Err(LOGIN_REMOTE_REQUIRED)
+    }

Also applies to: 149-153

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lore-client/src/cli/commands/auth.rs` at line 41, The LOGIN_REMOTE_REQUIRED
message in auth.rs should include the token-login fallback so users without a
remote know the valid remediation. Update the error path used by the auth
command, including the logic around the token-login branch in the authenticate
flow, to mention `--auth-url` alongside the existing remote URL or repository
remote guidance. Make sure the message still fits both the normal login path and
the `--token-type`/`--token` path handled by the auth command’s
login/authentication code.


#[derive(Args)]
pub struct AuthArgs {
#[command(subcommand)]
Expand Down Expand Up @@ -112,8 +114,58 @@ pub enum AuthCommands {
Clear,
}

fn configured_remote_url(repository_path: &str) -> Option<String> {
lore_revision::repository::load_repository_config(repository_path)
.ok()
.and_then(|config| config.remote_url)
.filter(|remote_url| !remote_url.is_empty())
}

fn login_with_token_can_use_explicit_auth_url(args: &AuthLoginArgs) -> bool {
args.token_type.is_some()
&& args.token.is_some()
&& args
.auth_url
.as_deref()
.is_some_and(|auth_url| !auth_url.is_empty())
}

fn resolve_login_remote_url(
globals: &LoreGlobalArgs,
args: &AuthLoginArgs,
) -> Result<LoreString, &'static str> {
if let Some(remote_url) = args
.remote_url
.as_deref()
.filter(|remote_url| !remote_url.is_empty())
{
return Ok(LoreString::from(remote_url));
}

if let Some(remote_url) = configured_remote_url(globals.repository_path.as_str()) {
return Ok(LoreString::from(remote_url.as_str()));
}

if login_with_token_can_use_explicit_auth_url(args) {
return Ok(LoreString::default());
}

Err(LOGIN_REMOTE_REQUIRED)
}

pub fn handle_login_command(globals: LoreGlobalArgs, args: &AuthLoginArgs) -> u8 {
let remote_url = LoreString::from(&args.remote_url);
if args.token_type.is_some() ^ args.token.is_some() {
crate::eprintln!("Both --token-type and --token are required for non-interactive login");
return 1;
}

let remote_url = match resolve_login_remote_url(&globals, args) {
Ok(remote_url) => remote_url,
Err(message) => {
crate::eprintln!("{message}");
return 1;
}
};

let callback = output_formatter().unwrap_or(Some(
(Box::new(move |event: &LoreEvent| match event {
Expand Down Expand Up @@ -148,9 +200,6 @@ pub fn handle_login_command(globals: LoreGlobalArgs, args: &AuthLoginArgs) -> u8
auth_url: args.auth_url.as_deref().into(),
};
runtime().block_on(auth::login_with_token(globals, args, callback)) as u8
} else if args.token_type.is_some() || args.token.is_some() {
crate::eprintln!("Both --token-type and --token are required for non-interactive login");
1
} else {
let args = LoreAuthLoginInteractiveArgs {
remote_url,
Expand Down Expand Up @@ -501,3 +550,74 @@ pub fn resolve_user_ids(
Err(_) => HashMap::default(),
}
}

#[cfg(test)]
mod tests {
use super::*;

fn globals_without_repo() -> LoreGlobalArgs {
let repository_path = std::env::temp_dir().join(format!(
"lore-auth-login-missing-repo-{}",
std::process::id()
));

LoreGlobalArgs {
repository_path: repository_path.display().to_string().into(),
..Default::default()
}
}

fn login_args(
remote_url: Option<&str>,
token_type: Option<&str>,
token: Option<&str>,
auth_url: Option<&str>,
) -> AuthLoginArgs {
AuthLoginArgs {
token_type: token_type.map(str::to_owned),
token: token.map(str::to_owned),
auth_url: auth_url.map(str::to_owned),
remote_url: remote_url.map(str::to_owned),
no_browser: false,
}
}

#[test]
fn interactive_login_without_remote_errors() {
let globals = globals_without_repo();
let args = login_args(None, None, None, None);

assert_eq!(
resolve_login_remote_url(&globals, &args).unwrap_err(),
LOGIN_REMOTE_REQUIRED
);
}

#[test]
fn token_login_with_auth_url_allows_missing_remote() {
let globals = globals_without_repo();
let args = login_args(
None,
Some("api-key"),
Some("token-value"),
Some("ucs-auth://auth.example.com"),
);

assert!(
resolve_login_remote_url(&globals, &args)
.unwrap()
.is_empty()
);
}

#[test]
fn login_uses_remote_argument_when_present() {
let globals = globals_without_repo();
let args = login_args(Some("lore.example.com"), None, None, None);

assert_eq!(
resolve_login_remote_url(&globals, &args).unwrap().as_str(),
"lore.example.com"
);
}
}
46 changes: 25 additions & 21 deletions lore/src/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use lore_credential::UserInfo;
use lore_error_set::prelude::*;
use lore_macro::LoreArgs;
use lore_revision::auth;
use lore_revision::auth::login::InteractiveLoginError;
use lore_revision::auth::login::LoginError;
use lore_revision::auth::userinfo::LoreAuthIdentityEventData;
use lore_revision::auth::userinfo::LoreAuthUserInfoEventData;
Expand All @@ -29,6 +30,8 @@ use crate::call::setup_execution;
use crate::call_delegation::dispatch_call;
use crate::interface::LoreString;

const LOGIN_REMOTE_REQUIRED: &str = "No remote URL supplied. Run `lore auth login <remote-url>` or run from a Lore repository with a configured remote.";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use a token-specific missing-endpoint error here.

This guard accepts either remote_url or auth_url, but the shared message only says the remote URL is missing. Return a token-specific message so API/CLI callers know auth_url is also a supported fix.

Suggested adjustment
 const LOGIN_REMOTE_REQUIRED: &str = "No remote URL supplied. Run `lore auth login <remote-url>` or run from a Lore repository with a configured remote.";
+const LOGIN_TOKEN_REMOTE_OR_AUTH_URL_REQUIRED: &str =
+    "No remote URL or auth URL supplied. Provide a remote URL, configured repository remote, or auth_url for token login.";
@@
-                return Err(LoginError::internal(LOGIN_REMOTE_REQUIRED));
+                return Err(LoginError::internal(LOGIN_TOKEN_REMOTE_OR_AUTH_URL_REQUIRED));

Also applies to: 214-215

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lore/src/auth.rs` at line 33, The shared missing-remote message in auth
handling is too generic for the guard that accepts either remote_url or
auth_url. Update the token-related branch in lore/src/auth.rs to use a
token-specific missing-endpoint message instead of LOGIN_REMOTE_REQUIRED, so
callers are told auth_url is also an acceptable fix; keep the same behavior for
the existing remote URL path and adjust the other referenced occurrence as well.


#[error_set]
pub enum AuthStoreError {
TokenNotFound,
Expand Down Expand Up @@ -206,19 +209,19 @@ async fn login_with_token_local(

let auth_url: Option<String> = args.auth_url.into();

LORE_CONTEXT
.scope(execution, async move {
let result = async move {
login_with_token_impl(
remote_url.as_str(),
args.token.as_str(),
args.token_type.as_str(),
auth_url.as_deref(),
)
.await
if let Err(err) = LORE_CONTEXT
.scope(execution.clone(), async move {
if remote_url.is_empty() && auth_url.as_deref().unwrap_or_default().is_empty() {
return Err(LoginError::internal(LOGIN_REMOTE_REQUIRED));
}
.await;
execution_context().dispatcher.complete_result(result).await

login_with_token_impl(
remote_url.as_str(),
args.token.as_str(),
args.token_type.as_str(),
auth_url.as_deref(),
)
.await
})
.await
}
Expand Down Expand Up @@ -291,15 +294,16 @@ async fn login_interactive_local(

let execution = setup_execution(globals, callback);

LORE_CONTEXT
.scope(execution, async move {
let result = async move {
match auth::login::interactive(remote_url.as_str(), args.no_browser != 0).await {
Ok(user_info) => {
send_user_info(user_info);
Ok(())
}
Err(err) => Err(err),
if let Err(err) = LORE_CONTEXT
.scope(execution.clone(), async move {
if remote_url.is_empty() {
return Err(InteractiveLoginError::internal(LOGIN_REMOTE_REQUIRED));
}

match auth::login::interactive(remote_url.as_str(), args.no_browser != 0).await {
Ok(user_info) => {
send_user_info(user_info);
Ok(())
}
}
.await;
Expand Down
Loading