Skip to content

chore(playwright): remove unnecessary global auth checks#8341

Merged
jmelahman merged 14 commits intomainfrom
jamison/rm-global-auth-checks
Feb 11, 2026
Merged

chore(playwright): remove unnecessary global auth checks#8341
jmelahman merged 14 commits intomainfrom
jamison/rm-global-auth-checks

Conversation

@jmelahman
Copy link
Copy Markdown
Contributor

@jmelahman jmelahman commented Feb 11, 2026

Description

Currently, there is a global check during setup that confirms that the admin, admin2, and user accounts can all login from the UI. We don't really need to check this -- we can just have the user creation pre-flight workflow fail in that case.

Moreover, replaces the auth.ts flow which performed browser-based login with an API-based for faster individual test running.

Also adds an explicit test for auth to cover the UI functionality spec (previously this was only assumed covered by these auth utils).

Seems to save about 2 min in CI for the admin playwright job 🚀

How Has This Been Tested?

npx playwright test create_and_edit_assistant.spec.ts

Takes about 16s locally now compared to 29s previously + has fewer 403 and similar error messages spamming the console.

Additional Options

  • [Required] I have considered whether this PR needs to be cherry-picked to the latest beta branch.
  • [Optional] Override Linear Check

Summary by cubic

Removed global UI auth checks and switched Playwright auth to API login for faster, quieter runs. Global setup now saves storage state via API and ensures a default public LLM provider; added explicit login UI tests.

  • Refactors

    • Global setup logs in via API request context and saves storage for admin, user, admin2; ensures a public LLM provider and sets it as default.
    • Auth utils use API; removed loginWithCredentials; loginAs and random-user flows call apiLogin (random user registers via API).
    • OnyxApiClient now takes APIRequestContext and optional baseUrl; lists via /admin/llm/provider; adds ensurePublicProvider and setProviderAsDefault.
    • Tests updated to pass page.request to OnyxApiClient; theme settings navigate before using localStorage, skip when Enterprise license is inactive, and guard cleanup when the form isn’t visible.
  • New Features

    • Login UI tests: fields render, valid login, invalid password, unknown user.
    • LLM provider fixture to create a public provider when missing, with cleanup.

Written for commit 92f0d3f. Summary will update on new commits.

@jmelahman jmelahman requested a review from a team as a code owner February 11, 2026 19:23
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 3 files

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Feb 11, 2026

Greptile Overview

Greptile Summary

This PR simplifies Playwright E2E authentication by removing browser-driven global auth checks and switching auth helpers to API-based login via Playwright request contexts.

  • web/tests/e2e/global-setup.ts now provisions users via /api/auth/register, logs in via /api/auth/login using request.newContext, and writes admin_auth.json / user_auth.json / admin2_auth.json storage states (still promoting admin2 via the manage API once admin storage state exists).
  • web/tests/e2e/utils/auth.ts refactors loginAs, loginWithCredentials, and loginAsRandomUser to authenticate via page.request rather than driving the login/signup UI, which speeds up tests and reduces console noise.
  • web/tests/e2e/auth/login.spec.ts adds explicit UI coverage for the login page rendering and basic success/failure behaviors, so the login UI remains tested even though most tests now authenticate via the API.

These changes fit the existing E2E testing approach where global-setup prepares reusable storageState files and individual specs either use test.use({ storageState: ... }) or call login utilities to quickly establish an authenticated context.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk.
  • Changes are confined to Playwright E2E test utilities and setup; the new API-based auth flow aligns with existing cookie-based auth via the Next.js /api proxy and storageState patterns, and adds explicit UI login coverage to avoid losing browser-login test coverage.
  • No files require special attention

Important Files Changed

Filename Overview
web/tests/e2e/auth/login.spec.ts Adds dedicated UI login flow tests (rendering, success, and failure cases) to explicitly cover browser-based auth.
web/tests/e2e/global-setup.ts Replaces browser-based login in global setup with request-context API login and storageState capture; keeps user provisioning and admin promotion flow.
web/tests/e2e/utils/auth.ts Refactors auth helpers to use API-based login/register via page.request, removing UI-driven flow and temporary debug logging.

Sequence Diagram

sequenceDiagram
    participant PW as Playwright Global Setup
    participant Req as APIRequestContext
    participant Next as Next.js /api proxy
    participant BE as Backend (FastAPI)

    PW->>Req: request.newContext({ baseURL })
    Req->>Next: POST /api/auth/register (admin/user/admin2)
    Next->>BE: POST /auth/register
    BE-->>Next: 200 or 400 REGISTER_USER_ALREADY_EXISTS
    Next-->>Req: response

    Req->>Next: POST /api/auth/login (form: username,password)
    Next->>BE: POST /auth/login
    BE-->>Next: 200 + Set-Cookie fastapiusersauth
    Next-->>Req: 200 + Set-Cookie
    Req->>Req: storageState({path: admin_auth.json})

    PW->>Req: request.newContext({ storageState: admin_auth.json })
    Req->>Next: PATCH /api/manage/set-user-role
    Next->>BE: PATCH /manage/set-user-role
    BE-->>Next: 200 (admin2 promoted)
    Next-->>Req: 200

    PW->>Req: Repeat login & storageState for user_auth.json and admin2_auth.json

    participant Test as Playwright Test (UI)
    participant Page as Browser Page

    Test->>Page: goto /auth/login, fill, click Sign In
    Page->>Next: POST /api/auth/login
    Next->>BE: POST /auth/login
    BE-->>Next: Set-Cookie fastapiusersauth
    Next-->>Page: Set-Cookie
    Test->>Page: expect URL /app
    Test->>Page: page.request.get(/api/me)
    Page->>Next: GET /api/me (Cookie)
    Next->>BE: GET /me
    BE-->>Next: 200 user info
    Next-->>Page: 200 user info
Loading

Copy link
Copy Markdown
Contributor

@justin-tahara justin-tahara left a comment

Choose a reason for hiding this comment

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

I'm pretty sure my comment is not an issue

await page.getByTestId("password").fill(password);
await page.getByRole("button", { name: "Sign In" }).click();

await expect(page).toHaveURL(/\/app/);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm sure this shouldn't be an issue anymore but we don't need to check for any /chat endpoints right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't need to. We could maybe add a test that asserts /chat redirects to /app for backwards compatibility, but not sure it'd belong here (or is really all that interesting).

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 11, 2026

Preview Deployment

Status Preview Commit Updated
https://onyx-preview-cudtc2d16-danswer.vercel.app 92f0d3f 2026-02-11 21:35:34 UTC

@jmelahman jmelahman force-pushed the jamison/rm-global-auth-checks branch from e9dbb7c to a37bfed Compare February 11, 2026 20:34
@jmelahman jmelahman enabled auto-merge February 11, 2026 20:54
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 15 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="web/tests/e2e/global-setup.ts">

<violation number="1" location="web/tests/e2e/global-setup.ts:215">
P2: OnyxApiClient ignores the Playwright-configured baseURL and always uses BASE_URL/localhost. With this new usage in global-setup, running tests against a non-default baseURL will still call localhost, so the public provider setup can fail or target the wrong backend. Consider passing the config baseURL into OnyxApiClient (or deriving it from the request context) so it uses the same host as the rest of global-setup.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@jmelahman jmelahman added this pull request to the merge queue Feb 11, 2026
Merged via the queue into main with commit 094d7a2 Feb 11, 2026
82 checks passed
@jmelahman jmelahman deleted the jamison/rm-global-auth-checks branch February 11, 2026 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants