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

# Auth Module Refactoring Implementation Plan

> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task.

**Goal:** Refactor `utils/auth.py` into a pure, decoupled authentication module with zero environment variable dependencies.

**Architecture:** Remove all environment variable reading from `auth.py`, make all parameters required, remove the `close_session()` wrapper function. Test scripts will explicitly load configuration and pass all parameters.

**Tech Stack:** Python 3.x, Playwright, pytest

---

## Task 1: Update utils/auth.py imports

**Files:**
- Modify: `utils/auth.py:1-23`

**Step 1: Remove unused imports**

Remove the following imports that are only used for environment variable access:
- `import os` (line 5)
- `from dotenv import load_dotenv` (line 10)
- `from pathlib import Path` (line 6)

The final imports should be:
```python
"""
Authentication module - Responsible for Yonyou BIP system login and logout operations
"""

from playwright.sync_api import Playwright, Browser, BrowserContext, Page, Frame
```

**Step 2: Remove module-level environment setup**

Delete lines 10-22 (BASE_DIR, load_dotenv, browsers_path configuration).

**Step 3: Verify syntax**

Run: `python -m py_compile utils/auth.py`
Expected: No syntax errors (function definitions will fail later steps, that's OK)

**Step 4: Commit**

```bash
git add utils/auth.py
git commit -m "refactor: remove environment variable imports from auth module

- Remove os, dotenv, and pathlib imports
- Remove module-level environment setup code
- Prepare for pure function implementation

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>"
```

---

## Task 2: Refactor login() function signature

**Files:**
- Modify: `utils/auth.py:25-52`

**Step 1: Update function signature to remove default parameters**

Change the `login()` function signature from:
```python
def login(
    playwright: Playwright,
    username: str = None,
    password: str = None,
    url: str = None,
    headless: bool = None,
    ignore_https_errors: bool = None,
    verbose: bool = True,
) -> tuple[Browser, BrowserContext, Page, Frame]:
```

To:
```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]:
```

**Step 2: Update docstring**

Replace the docstring (lines 34-52) with:
```python
    """
    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)
            - browser: Browser instance
            - context: Browser context
            - page: Page object
            - main_frame: Main iframe after login (forwardFrame)
    """
```

**Step 3: Verify syntax**

Run: `python -m py_compile utils/auth.py`
Expected: No syntax errors

**Step 4: Commit**

```bash
git add utils/auth.py
git commit -m "refactor: update login() signature to require all parameters

- Remove default values for username, password, url, headless, ignore_https_errors
- Update docstring to reflect required parameters
- Maintain backward compatibility with 4-tuple return value

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>"
```

---

## Task 3: Remove environment variable logic from login() body

**Files:**
- Modify: `utils/auth.py:53-77`

**Step 1: Remove environment variable reading code**

Delete lines 56-76 that contain:
- `username = username or os.getenv("ERP_USERNAME")`
- `password = password or os.getenv("ERP_PASSWORD")`
- URL construction logic with `os.getenv("ERP_URL")`
- headless default handling with `os.getenv("ERP_HEADLESS")`
- ignore_https_errors default handling with `os.getenv("ERP_IGNORE_HTTPS_ERRORS")`

**Step 2: Add parameter validation**

After the imports section (after the docstring), add validation:
```python
    # Validate required parameters
    if not username:
        raise ValueError("username is required")
    if not password:
        raise ValueError("password is required")
    if not url:
        raise ValueError("url is required")
```

**Step 3: Verify the login() function body**

The function body should now start directly with:
```python
    import time

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

    # Launch browser
    browser = playwright.chromium.launch(headless=headless)
    # ... rest of function unchanged
```

**Step 4: Verify syntax**

Run: `python -m py_compile utils/auth.py`
Expected: No syntax errors

**Step 5: Commit**

```bash
git add utils/auth.py
git commit -m "refactor: remove environment variable reading from login()

- Remove all os.getenv() calls from login() function
- Remove URL construction logic (caller provides complete URL)
- Add parameter validation with clear error messages
- Function is now pure with no side effects

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>"
```

---

## Task 4: Remove close_session() function

**Files:**
- Modify: `utils/auth.py:161-177`

**Step 1: Delete the close_session() function**

Remove lines 161-177, the entire `close_session()` function:
```python
def close_session(browser: Browser, context: BrowserContext) -> None:
    """
    Close browser session

    Args:
        browser: Browser instance
        context: Browser context
    """
    # Check if auto-close browser is enabled
    auto_close = os.getenv("ERP_AUTO_CLOSE_BROWSER", "true").lower() in ("true", "1", "yes")

    if auto_close:
        context.close()
        browser.close()
    else:
        print("Note: Browser not auto-closed (ERP_AUTO_CLOSE_BROWSER=false)")
```

**Step 2: Verify auth.py still has only two functions**

Run: `grep "^def " utils/auth.py`
Expected output:
```
def login(
def logout(
```

**Step 3: Verify syntax**

Run: `python -m py_compile utils/auth.py`
Expected: No syntax errors

**Step 4: Commit**

```bash
git add utils/auth.py
git commit -m "refactor: remove close_session() function

- Remove close_session() wrapper function entirely
- Callers now directly manage browser/context lifecycle
- Reduces module to pure authentication functions only

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>"
```

---

## Task 5: Update tests/test_login.py to use new API

**Files:**
- Modify: `tests/test_login.py:13,37-39,62-69`

**Step 1: Update import line**

The import on line 13 stays the same:
```python
from utils.auth import login, logout, close_session
```

Change to:
```python
from utils.auth import login, logout
```

**Step 2: Add URL construction before login() call**

After line 31 (after displaying configuration), add URL construction:
```python
# Construct complete login URL
url = f"{os.getenv('ERP_URL').rstrip('/')}/yonbip/resources/uap/rbac/login/main/index.html"
```

**Step 3: Update login() call with all required parameters**

Replace lines 37-40:
```python
        browser, context, page, main_frame = login(
            playwright=p,
            verbose=True
        )
```

With:
```python
        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
        )
```

**Step 4: Replace close_session() with direct calls**

Replace lines 62-69:
```python
        # Check if auto-close browser
        auto_close = os.getenv("ERP_AUTO_CLOSE_BROWSER", "true").lower() in ("true", "1", "yes")
        if not auto_close:
            print("\n[INFO] Browser will stay open for 10 seconds for manual observation...")
            time.sleep(10)

        # Close browser
        print("\n[CLEANUP] Closing browser session...")
        close_session(browser, context)
```

With:
```python
        # Check if auto-close browser
        auto_close = os.getenv("ERP_AUTO_CLOSE_BROWSER", "true").lower() in ("true", "1", "yes")
        if not auto_close:
            print("\n[INFO] Browser will stay open for 10 seconds for manual observation...")
            time.sleep(10)

        # Close browser
        print("\n[CLEANUP] Closing browser session...")
        context.close()
        browser.close()
```

**Step 5: Verify syntax**

Run: `python -m py_compile tests/test_login.py`
Expected: No syntax errors

**Step 6: Commit**

```bash
git add tests/test_login.py
git commit -m "refactor: update test_login.py to use new auth API

- Import only login and logout (remove close_session)
- Add explicit URL construction before login call
- Pass all required parameters to login()
- Replace close_session() with direct context.close() and browser.close()

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>"
```

---

## Task 6: Run tests to verify functionality

**Files:**
- Test: `tests/test_login.py`, `utils/auth.py`

**Step 1: Activate virtual environment**

Run: `source .venv/Scripts/activate`
Expected: Command prompt shows `(.venv)`

**Step 2: Run login test**

Run: `python tests/test_login.py`
Expected: Test completes successfully with login/logout operations

**Step 3: Verify no regressions**

Check that the test output shows:
- Configuration display
- Successful login
- Successful logout
- Browser closes cleanly

**Step 4: Run auth config test**

Run: `python tests/test_auth_config.py`
Expected: Configuration check completes (no changes needed in this file)

**Step 5: Commit if any fixes needed**

If tests revealed issues:
```bash
git add tests/test_login.py utils/auth.py
git commit -m "fix: address test failures in refactored auth module

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>"
```

If all tests pass, no commit needed for this step.

---

## Task 7: Update CLAUDE.md documentation

**Files:**
- Modify: `CLAUDE.md:24-35,41-46`

**Step 1: Update Environment Configuration section**

Replace lines 24-35 with:
```markdown
## Environment Configuration

Configuration is loaded from `.env` file in the project root. Never commit `.env` to version control - use `.env.example` as a template.

**Environment Variables:**
- `PLAYWRIGHT_BROWSERS_PATH` - Path to Playwright browser installation
- `ERP_URL` - Base URL for the ERP system
- `ERP_USERNAME` - Login username
- `ERP_PASSWORD` - Login password
- `ERP_HEADLESS` - Whether to run browser in headless mode (true/false)
- `ERP_IGNORE_HTTPS_ERRORS` - Whether to ignore HTTPS certificate errors (true/false)

Note: Test scripts are responsible for loading environment variables and passing configuration to utility functions.
```

**Step 2: Update Module Structure section**

Replace lines 41-46 with:
```markdown
**`utils/auth.py`** - Core authentication module (pure functions)
- `login()` - Handles Yonyou BIP login with automatic force-login popup detection. Requires all parameters (username, password, url, headless, ignore_https_errors).
- `logout()` - Performs logout with confirmation dialog handling.
- Returns tuple: `(browser, context, page, main_frame)` where `main_frame` is the forwardFrame iframe.
- Callers are responsible for browser lifecycle management (context.close(), browser.close()).
```

**Step 3: Add URL Construction Pattern**

After line 52 (after Page Interaction Pattern section), add:
```markdown
### URL Construction Pattern

Callers must construct the complete login URL before passing to `login()`:

```python
url = f"{os.getenv('ERP_URL').rstrip('/')}/yonbip/resources/uap/rbac/login/main/index.html"
```
```

**Step 4: Update Code Conventions section**

Replace line 84 (Environment-First convention) with:
```markdown
2. **Environment-First**: Test scripts load environment variables and explicitly pass configuration to utility functions. No hardcoded values in code.
```

**Step 5: Verify documentation**

Run: `grep -n "os.getenv" CLAUDE.md`
Expected: Only in code examples, not as recommendations for module internals

**Step 6: Commit**

```bash
git add CLAUDE.md
git commit -m "docs: update CLAUDE.md for refactored auth module

- Clarify that auth module is pure with no environment access
- Document URL construction pattern for callers
- Update module structure documentation
- Emphasize caller responsibility for configuration

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>"
```

---

## Task 8: Create .env.example template

**Files:**
- Create: `.env.example`

**Step 1: Create environment variable template**

Create `.env.example` with:
```bash
# Playwright Configuration
PLAYWRIGHT_BROWSERS_PATH=path/to/playwright/browsers

# ERP System Configuration
ERP_URL=https://your-erp-system.com
ERP_USERNAME=your_username
ERP_PASSWORD=your_password

# Browser Behavior
ERP_HEADLESS=false
ERP_IGNORE_HTTPS_ERRORS=true
```

**Step 2: Verify .env is in .gitignore**

Run: `grep .env .gitignore`
Expected: `.env` is listed

**Step 3: Commit**

```bash
git add .env.example
git commit -m "docs: add .env.example template

- Provide template for required environment variables
- Document all configuration options
- Help users set up their local environment

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>"
```

---

## Task 9: Final verification and cleanup

**Files:**
- Verify: `utils/auth.py`, `tests/`, `CLAUDE.md`

**Step 1: Verify no environment imports in auth.py**

Run: `grep -E "(os\.getenv|load_dotenv)" utils/auth.py`
Expected: No matches

**Step 2: Verify all parameters are required**

Run: `grep "def login" utils/auth.py`
Expected: Function signature has no `= None` defaults except `verbose=True`

**Step 3: Run all tests**

Run: `python tests/test_login.py && python tests/test_auth_config.py`
Expected: All tests pass

**Step 4: Check documentation consistency**

Run: `grep -n "close_session" CLAUDE.md`
Expected: No mentions (function was removed)

**Step 5: Final commit if needed**

If any cleanup was done:
```bash
git add -A
git commit -m "chore: final cleanup for auth module refactoring

- Verify no environment dependencies remain
- Confirm all tests pass
- Documentation is consistent

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>"
```

**Step 6: Push to remote**

Run: `git push origin master`
Expected: All commits pushed successfully

---

## Summary

This refactoring achieves:
1. **Pure Functions**: `auth.py` has zero side effects or environment dependencies
2. **Explicit Dependencies**: All parameters required, no hidden behavior
3. **Separation of Concerns**: Configuration managed by callers, auth module handles business logic only
4. **Backward Compatible**: 4-tuple return value maintained for existing code
5. **Tested**: All tests updated and passing

**Migration Impact**: Low - only test scripts need updates, API changes are additive (making optional params required)