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

```python
# 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:**
```python
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:**
```python
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:
```python
# 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

```python
# 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
```python
# 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()`
```python
# 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
```python
# DELETE THIS ENTIRE FUNCTION
def close_session(browser: Browser, context: BrowserContext) -> None:
    ...
```

#### Code to Modify

`login()` signature - remove defaults:
```python
# 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:
```python
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):**
```python
with sync_playwright() as p:
    browser, context, page, main_frame = login(
        playwright=p,
        verbose=True
    )
    # ... use browser ...
    close_session(browser, context)
```

**After:**
```python
# 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