cli+tests: improve PAT handling logic + error handling

from internal code review

Author: RMCrean

docs/user-guides/using-divbase-programmatically.md

@@ -12,9 +12,9 @@ See [Account Management — Personal Access Tokens](./account-management.md#pers
 
 You can store/use a PAT in two ways
 
-### Option 1 — Let `divbase-cli` handle storing the PAT (recommended)
+### (Recommended) — Let `divbase-cli` handle storing the PAT
 
-After creating a PAT on the website, store it on your device by copy pasting the pre-filled commands shown on the website, it will look something like this:
+After creating a PAT on the DivBase website, store it on your device by copy pasting the pre-filled commands shown on the website, it will look something like this:
 
 ```bash
 # with an expiry date:
@@ -23,7 +23,7 @@ divbase-cli auth add-pat "my-pat-name" --expires UNIX_TIMESTAMP
 divbase-cli auth add-pat "my-pat-name"
 ```
 
-You will be prompted to paste the token value. It is then stored securely in your OS keyring (or in a restricted file if no keyring is available) and used automatically on every subsequent `divbase-cli` command.
+You will be prompted to paste the token value. It will then be stored securely in your OS keyring (or in a restricted file if no keyring is available) and used automatically on every subsequent `divbase-cli` command.
 
 ```bash
 divbase-cli auth pat-info   # show name and expiry of the stored PAT
@@ -33,7 +33,7 @@ divbase-cli auth rm-pat     # remove the stored PAT from this device
 !!! info "This strategy works on HPC clusters"
     On the login node with divbase-cli [installed as we recommend](./installation.md), store the PAT as described above. The token will be available to all your jobs running on the cluster, without needing to worry about setting any environment variables in your job scripts.
 
-### Option 2 — Environment variable
+### (Not recommended alternative) — Store the PAT in an environment variable
 
 Set `DIVBASE_API_PAT` in your shell or job script:
 
@@ -44,8 +44,7 @@ divbase-cli files ls
 
 You may prefer this if you want explicit per-job control of which token is used.
 
-1. Store it securely on your device using `divbase-cli auth add-pat` (recommended and works on HPC clusters!)
-2. Set it as an environment variable `DIVBASE_API_PAT` (can give you more control over which token is used when)
+---
 
 !!! question "What if I have both an active login session and a personal access token set?"
     `divbase-cli` follows this priority order:

packages/divbase-cli/src/divbase_cli/cli_commands/auth_cli.py

@@ -3,6 +3,7 @@
 """
 
 import logging
+import time
 from datetime import datetime
 
 import typer
@@ -14,6 +15,7 @@
 from divbase_cli.config_resolver import resolve_url_for_non_project_specific_commands
 from divbase_cli.services.announcements import get_and_display_announcements
 from divbase_cli.user_auth import (
+    PERSONAL_ACCESS_TOKEN_EXPIRED_MESSAGE,
     PATData,
     check_existing_session,
     delete_stored_pat,
@@ -92,6 +94,7 @@ def add_pat(
         "--expires",
         "-e",
         help="When the personal access token (PAT) expires (if it does), as a unix timestamp.",
+        min=1,
     ),
     overwrite_existing: bool = typer.Option(
         False, "--overwrite-existing", "-o", help="Overwrite the existing stored PAT if one already exists."
@@ -114,7 +117,7 @@ def add_pat(
         )
         raise typer.Exit(code=1)
 
-    if expires_unix_timestamp and expires_unix_timestamp < int(datetime.now().timestamp()):
+    if expires_unix_timestamp and expires_unix_timestamp < time.time():
         print("The expiry time you entered is in the past. Please enter a valid expiry time for the PAT.")
         raise typer.Exit(code=1)
 
@@ -156,12 +159,13 @@ def pat_info():
         print("No personal access token stored on this device.")
         return
 
-    if pat_data.pat_expires_at and pat_data.pat_expires_at <= datetime.now().timestamp():
-        print("[red]Warning: this PAT has expired.[/red]")
-
     print(f"Name: {pat_data.name}")
     print(f"Expires: {pat_data.pat_expiry_formatted()}")
 
+    if pat_data.is_pat_expired():
+        print("[red bold]Warning [/red bold]")
+        print(PERSONAL_ACCESS_TOKEN_EXPIRED_MESSAGE)
+
 
 @auth_app.command("whoami")
 def whoami():

packages/divbase-cli/src/divbase_cli/config_resolver.py

@@ -10,7 +10,7 @@
 
 from divbase_cli.cli_config import cli_settings
 from divbase_cli.cli_exceptions import AuthenticationError, ProjectNameNotSpecifiedError
-from divbase_cli.user_auth import load_stored_user_pat
+from divbase_cli.user_auth import get_pat_for_authentication
 from divbase_cli.user_config import ProjectConfig, load_user_config
 
 
@@ -28,7 +28,7 @@ def ensure_logged_in(desired_url: str | None = None) -> str:
             )
         return config.logged_in_url
 
-    if cli_settings.DIVBASE_API_PAT or load_stored_user_pat():
+    if get_pat_for_authentication():
         return desired_url or cli_settings.DIVBASE_API_URL
 
     raise AuthenticationError("You are not logged in. Please log in with 'divbase-cli auth login [EMAIL]'.")
@@ -48,8 +48,7 @@ def resolve_url_for_non_project_specific_commands() -> str:
     if config.logged_in_url:
         return config.logged_in_url
 
-    # No active session — fall back to PAT if available.
-    if cli_settings.DIVBASE_API_PAT or load_stored_user_pat():
+    if get_pat_for_authentication():
         return cli_settings.DIVBASE_API_URL
 
     raise AuthenticationError("You are not logged in. Please log in with 'divbase-cli auth login [EMAIL]'.")

packages/divbase-cli/src/divbase_cli/user_auth.py

@@ -21,6 +21,7 @@
 from datetime import datetime
 from json import JSONDecodeError
 from pathlib import Path
+from typing import Any
 
 import httpx
 import keyring
@@ -38,6 +39,12 @@
 from divbase_lib.divbase_constants import CLI_VERSION_HEADER_KEY
 
 LOGIN_AGAIN_MESSAGE = "Your session has expired. Please log in again with 'divbase-cli auth login [EMAIL]'."
+PERSONAL_ACCESS_TOKEN_EXPIRED_MESSAGE = (
+    "Your Personal Access Token (PAT) has expired. \n"
+    "Please create a new one and add it with 'divbase-cli auth add-pat'. \n"
+    "See https://scilifelabdatacentre.github.io/divbase/user-guides/account-management/#personal-access-tokens for more details."
+)
+
 
 logger = logging.getLogger(__name__)
 
@@ -59,25 +66,11 @@ def dump_tokens(self, fallback_output_path: Path = cli_settings.TOKENS_FALLBACK_
             "access_token_expires_at": self.access_token_expires_at,
             "refresh_token_expires_at": self.refresh_token_expires_at,
         }
-        try:
-            keyring.set_password(
-                service_name=cli_settings.KEYRING_SERVICE,
-                username=cli_settings.KEYRING_TOKENS_USERNAME,
-                password=json.dumps(token_dict),
-            )
-            fallback_output_path.unlink(missing_ok=True)
-            logger.debug("JWTs stored in device keyring successfully.")
-            return
-        except KeyringError as e:
-            logger.debug(f"Keyring JWT storage failed with error: {e}\n falling back to file storage.")
-
-        fallback_output_path.parent.mkdir(parents=True, exist_ok=True)
-        # Create file with 0600 permissions (user read/write only).
-        # Windows doesn't support 0600 permissions, but Windows can use keyring, so should never reach this fallback.
-        # Even if a Windows OS can't use keyring the fallback will still work, the 0600 mode will just be ignored.
-        fd = os.open(path=fallback_output_path, flags=os.O_WRONLY | os.O_CREAT | os.O_TRUNC, mode=0o600)
-        with os.fdopen(fd, "w") as file:
-            yaml.safe_dump(token_dict, file, sort_keys=False)
+        _dump_to_keyring_or_file(
+            data=token_dict,
+            keyring_user_name=cli_settings.KEYRING_TOKENS_USERNAME,
+            fallback_file_path=fallback_output_path,
+        )
 
     def is_access_token_expired(self) -> bool:
         """Check if the access token is expired"""
@@ -112,25 +105,16 @@ def dump_pat_data(self, fallback_output_path: Path = cli_settings.PATS_FALLBACK_
             "pat_expires_at": self.pat_expires_at,
             "pat": self.pat.get_secret_value(),
         }
-        try:
-            keyring.set_password(
-                service_name=cli_settings.KEYRING_SERVICE,
-                username=cli_settings.KEYRING_PATS_USERNAME,
-                password=json.dumps(token_dict),
-            )
-            fallback_output_path.unlink(missing_ok=True)
-            logger.debug("PATs stored in device keyring successfully.")
-            return
-        except KeyringError as e:
-            logger.debug(f"Keyring PAT storage failed with error: {e}\n falling back to file storage.")
-
-        fallback_output_path.parent.mkdir(parents=True, exist_ok=True)
-        # Create file with 0600 permissions (user read/write only).
-        # Windows doesn't support 0600 permissions, but Windows can use keyring, so should never reach this fallback.
-        # Even if a Windows OS can't use keyring the fallback will still work, the 0600 mode will just be ignored.
-        fd = os.open(path=fallback_output_path, flags=os.O_WRONLY | os.O_CREAT | os.O_TRUNC, mode=0o600)
-        with os.fdopen(fd, "w") as file:
-            yaml.safe_dump(token_dict, file, sort_keys=False)
+        _dump_to_keyring_or_file(
+            data=token_dict,
+            keyring_user_name=cli_settings.KEYRING_PATS_USERNAME,
+            fallback_file_path=fallback_output_path,
+        )
+
+    def is_pat_expired(self) -> bool:
+        """Check if the PAT is expired"""
+        cutoff = time.time() + 10  # 10 second buffer to account for clock skew or time delay
+        return self.pat_expires_at is not None and self.pat_expires_at <= cutoff
 
     def pat_expiry_formatted(self) -> str:
         """Returns the token's expiry time in a human readable format in user's current timezone."""
@@ -314,7 +298,7 @@ def load_user_tokens(token_path: Path = cli_settings.TOKENS_FALLBACK_PATH) -> To
             access_token_expires_at=token_dict["access_token_expires_at"],
             refresh_token_expires_at=token_dict["refresh_token_expires_at"],
         )
-    except KeyError as e:
+    except (KeyError, TypeError) as e:
         logger.debug(f"User tokens file at {token_path} appears to be malformed: {e}")
         return None
 
@@ -360,7 +344,7 @@ def load_stored_user_pat(pat_fallback_path: Path = cli_settings.PATS_FALLBACK_PA
             pat=SecretStr(pat_dict["pat"]),
             pat_expires_at=pat_dict["pat_expires_at"],
         )
-    except KeyError as e:
+    except (KeyError, TypeError) as e:
         logger.debug(f"User PAT file at {pat_fallback_path} appears to be malformed: {e}")
         return None
 
@@ -386,12 +370,8 @@ def get_pat_for_authentication() -> SecretStr | None:
     if not pat_data:
         return None
 
-    if pat_data.pat_expires_at and pat_data.pat_expires_at <= time.time():
-        raise AuthenticationError(
-            "Your Personal Access Token (PAT) has expired. \n"
-            "Please create a new one and add it with 'divbase-cli auth add-pat'. \n"
-            "See https://scilifelabdatacentre.github.io/divbase/user-guides/account-management/#personal-access-tokens for more details."
-        )
+    if pat_data.is_pat_expired():
+        raise AuthenticationError(PERSONAL_ACCESS_TOKEN_EXPIRED_MESSAGE)
 
     return pat_data.pat
 
@@ -515,3 +495,31 @@ def _refresh_access_token(token_data: TokenData, divbase_base_url: str) -> Token
     )
     new_token_data.dump_tokens()
     return new_token_data
+
+
+def _dump_to_keyring_or_file(data: dict[Any, Any], keyring_user_name: str, fallback_file_path: Path) -> None:
+    """
+    Helper fn to dump a dict to user's OS keyring.
+
+    If dump fails, will fallback to a local file and set permissions to 0600 (user read/write only).
+    Used to store both JWTs and PATs.
+    """
+    try:
+        keyring.set_password(
+            service_name=cli_settings.KEYRING_SERVICE,
+            username=keyring_user_name,
+            password=json.dumps(data),
+        )
+        fallback_file_path.unlink(missing_ok=True)
+        logger.debug(f"Data stored in user device keyring under username: {keyring_user_name}")
+        return
+    except KeyringError as e:
+        logger.debug(f"Keyring storage failed with error: {e}\n falling back to file storage.")
+
+    fallback_file_path.parent.mkdir(parents=True, exist_ok=True)
+    # Create file with 0600 permissions (user read/write only).
+    # Windows doesn't support 0600 permissions, but Windows can use keyring, so should never reach this fallback.
+    # Even if a Windows OS can't use keyring the fallback will still work, the 0600 mode will just be ignored.
+    fd = os.open(path=fallback_file_path, flags=os.O_WRONLY | os.O_CREAT | os.O_TRUNC, mode=0o600)
+    with os.fdopen(fd, "w") as file:
+        yaml.safe_dump(data, file, sort_keys=False)

tests/e2e_integration/cli_commands/test_auth_cli.py

@@ -16,6 +16,7 @@
 from divbase_cli.cli_exceptions import AuthenticationError, DivBaseAPIConnectionError, DivBaseAPIError
 from divbase_cli.divbase_cli import app
 from divbase_cli.user_auth import LOGIN_AGAIN_MESSAGE
+from divbase_lib.divbase_constants import PAT_TOKEN_PREFIX
 
 runner = CliRunner()
 
@@ -279,15 +280,15 @@ def test_jwt_session_takes_priority_over_pat(disable_keyring_backend, monkeypatc
     """
     log_in_as_user()
 
-    monkeypatch.setattr(cli_settings, "DIVBASE_API_PAT", SecretStr("divbase_pat_fake_not_a_real_token"))
+    monkeypatch.setattr(cli_settings, "DIVBASE_API_PAT", SecretStr(f"{PAT_TOKEN_PREFIX}_fake_not_a_real_token"))
 
     result = runner.invoke(app=app, args="auth whoami")
     assert result.exit_code == 0
     assert USER_EMAIL in result.stdout
 
 
 _FAKE_PAT_NAME = "work-laptop"
-_FAKE_PAT = "divbase_pat_fakefakefakefakefakefakefake"
+_FAKE_PAT = f"{PAT_TOKEN_PREFIX}_fakefakefakefakefakefakefake"
 
 
 def run_add_pat_cmd(

tests/e2e_integration/playwright/test_personal_access_tokens.py

@@ -86,7 +86,7 @@ def test_new_pat_form_renders(logged_in_edit_user_pat_page: Page):
 def test_create_full_access_pat_shows_token(logged_in_edit_user_pat_page: Page):
     """
     Creating a PAT with no scope restrictions shows the 'Token generated' page,
-    the copy-now warning, and the raw token (which must start with 'divbase_pat_').
+    the copy-now warning, and the raw token (which must start with PAT_TOKEN_PREFIX set in divbase-lib's constants module).
     """
     fill_and_submit_new_pat_form(logged_in_edit_user_pat_page, name="my-hpc-token")
 

tests/unit/divbase_api/test_security.py

@@ -17,6 +17,7 @@
     verify_password,
     verify_token,
 )
+from divbase_lib.divbase_constants import PAT_TOKEN_PREFIX
 
 USER_ID = 1
 
@@ -121,7 +122,7 @@ def test_password_hashing():
 
 def test_personal_access_token_hashing():
     """Test that hashing a personal access token produces a consistent hash."""
-    raw_token = SecretStr("divbase_pat_abc123")
+    raw_token = SecretStr(f"{PAT_TOKEN_PREFIX}_abc123")
     hashed_token_1 = hash_personal_access_token(raw_token)
     hashed_token_2 = hash_personal_access_token(raw_token)
     assert hashed_token_1 == hashed_token_2