BIPAuto/docs/plans/2026-03-27-auth-refactoring-design.md

Auth Module Refactoring Design

Date: 2026-03-27 Status: Approved Author: Claude Code

Overview

Refactor the utils/auth.py module to follow decoupling principles by removing all environment variable dependencies and making it a pure, stateless authentication component.

Problem Statement

Current Issues

The utils/auth.py module has tight coupling to environment configuration:

1. Module-level side effects: Loads environment variables on import (lines 10-22)
2. Implicit defaults: login() function reads from environment variables when parameters are None
3. Hidden behavior: URL construction and browser path configuration embedded in the module
4. Mixed responsibilities: Authentication logic mixed with configuration management

Design Principles Violated

• Separation of Concerns: Configuration and business logic are mixed
• Dependency Inversion: Module depends on concrete environment implementation
• Single Responsibility: Module handles both auth and configuration loading

Proposed Solution

Architecture

Transform utils/auth.py into a pure, stateless authentication module with:

• Zero environment variable dependencies
• All parameters required (no defaults from environment)
• No module-level initialization or side effects
• Explicit parameter passing only

Module Structure

# utils/auth.py - Pure Authentication Module

# Removed:
# - Environment variable loading (load_dotenv)
# - BASE_DIR and path calculation
# - os.getenv() calls
# - close_session() function

# Functions:
# login(playwright, username, password, url, headless, ignore_https_errors, verbose)
# logout(main_frame, verbose)

API Changes

login() Function

Before:

def login(
    playwright: Playwright,
    username: str = None,  # Defaults to os.getenv("ERP_USERNAME")
    password: str = None,  # Defaults to os.getenv("ERP_PASSWORD")
    url: str = None,       # Defaults to os.getenv("ERP_URL") + path
    headless: bool = None, # Defaults to os.getenv("ERP_HEADLESS")
    ignore_https_errors: bool = None,  # Defaults to os.getenv("ERP_IGNORE_HTTPS_ERRORS")
    verbose: bool = True,
) -> tuple[Browser, BrowserContext, Page, Frame]:

After:

def login(
    playwright: Playwright,
    username: str,
    password: str,
    url: str,
    headless: bool,
    ignore_https_errors: bool,
    verbose: bool = True,
) -> tuple[Browser, BrowserContext, Page, Frame]:
    """
    Login to Yonyou BIP system

    Args:
        playwright: Playwright instance
        username: Username (required)
        password: Password (required)
        url: Complete login page URL (required)
        headless: Whether to use headless mode (required)
        ignore_https_errors: Whether to ignore HTTPS errors (required)
        verbose: Whether to print detailed logs (default: True)

    Returns:
        tuple: (browser, context, page, main_frame)
    """

Changes:

• All parameters become required (no defaults)
• URL parameter expects complete URL (no automatic path appending)
• Removed lines 59-76 (environment variable reading)
• Removed lines 62-63 (URL construction logic)

logout() Function

No changes - already a pure function.

close_session() Function

REMOVED ENTIRELY

Callers now manage browser lifecycle directly:

# Caller code
context.close()
browser.close()

Data Flow

Before:

Test Script → login() → [reads .env internally] → Browser
                  ↑
            (implicit config)

After:

Test Script → [loads .env] → [constructs URL] → login() → Browser
                  ↓                      ↓
            (explicit config)    (explicit params)

Implementation Details

Imports to Remove

# Remove these imports:
import os  # Only used for os.getenv()
from dotenv import load_dotenv  # No longer needed
from pathlib import Path  # Only used for BASE_DIR calculation

Code to Remove

Lines 10-22: Environment setup

# DELETE THESE LINES:
# Load environment variables
from dotenv import load_dotenv

# Get project root directory
BASE_DIR = Path(__file__).resolve().parent.parent

# Load .env file
env_path = BASE_DIR / ".env"
load_dotenv(env_path)

# Configure Playwright browser path
browsers_path = os.getenv("PLAYWRIGHT_BROWSERS_PATH")
if browsers_path:
    os.environ["PLAYWRIGHT_BROWSERS_PATH"] = browsers_path

Lines 59-76: Environment variable defaults in login()

# DELETE THESE LINES:
# URL handling: read from environment variable or parameter
if not url:
    url = os.getenv("ERP_URL")
    if url and not url.endswith("login/main/index.html"):
        url = url.rstrip("/") + "/yonbip/resources/uap/rbac/login/main/index.html"

if not url:
    raise ValueError("URL must be provided either as parameter or through ERP_URL environment variable")

# headless parameter handling
if headless is None:
    headless_str = os.getenv("ERP_HEADLESS", "false").lower()
    headless = headless_str in ("true", "1", "yes")

# ignore_https_errors parameter handling
if ignore_https_errors is None:
    ignore_https_errors_str = os.getenv("ERP_IGNORE_HTTPS_ERRORS", "true").lower()
    ignore_https_errors = ignore_https_errors_str in ("true", "1", "yes")

Lines 161-177: close_session() function

# DELETE THIS ENTIRE FUNCTION
def close_session(browser: Browser, context: BrowserContext) -> None:
    ...

Code to Modify

login() signature - remove defaults:

# CHANGE FROM:
def login(playwright, username=None, password=None, url=None, headless=None, ignore_https_errors=None, verbose=True)

# CHANGE TO:
def login(playwright, username, password, url, headless, ignore_https_errors, verbose=True)

Add parameter validation:

if not username:
    raise ValueError("username is required")
if not password:
    raise ValueError("password is required")
if not url:
    raise ValueError("url is required")

Testing Impact

Test Script Changes

All test scripts must be updated to:

1. Load environment variables explicitly (already done)
2. Construct complete URLs before calling login()
3. Pass all parameters explicitly to login()
4. Replace close_session() calls with direct context.close() and browser.close()

Example Migration

Before (test_login.py):

with sync_playwright() as p:
    browser, context, page, main_frame = login(
        playwright=p,
        verbose=True
    )
    # ... use browser ...
    close_session(browser, context)

After:

# Load and prepare config
url = f"{os.getenv('ERP_URL').rstrip('/')}/yonbip/resources/uap/rbac/login/main/index.html"

with sync_playwright() as p:
    browser, context, page, main_frame = login(
        playwright=p,
        username=os.getenv('ERP_USERNAME'),
        password=os.getenv('ERP_PASSWORD'),
        url=url,
        headless=os.getenv('ERP_HEADLESS', 'false').lower() in ('true', '1', 'yes'),
        ignore_https_errors=os.getenv('ERP_IGNORE_HTTPS_ERRORS', 'true').lower() in ('true', '1', 'yes'),
        verbose=True
    )
    # ... use browser ...
    context.close()
    browser.close()

Test Files to Update

• tests/test_login.py - Update login() calls and close_session() usage
• tests/test_auth_config.py - No changes (doesn't call login())

Benefits

1. Decoupling: Auth module independent of configuration source
2. Testability: Easier to test with mock data
3. Clarity: Explicit dependencies make data flow obvious
4. Flexibility: Can be used with any configuration source (env, config file, CLI args, etc.)
5. Purity: Functions have no hidden side effects

Migration Path

1. Update utils/auth.py with all changes
2. Update tests/test_login.py to use new API
3. Run tests to verify functionality
4. Update documentation (CLAUDE.md)

Risks and Mitigations

Risk	Mitigation
Breaking existing scripts	Clear migration guide; tests updated first
Parameter verbosity	Tests already have env loading code
URL construction duplication	Document pattern in CLAUDE.md

Acceptance Criteria

• utils/auth.py has zero os.getenv() calls
• utils/auth.py has no load_dotenv() calls
• login() requires all parameters (no None defaults)
• close_session() function removed
• All tests pass with new API
• CLAUDE.md documentation updated