Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(server): improve confusing login errors #13372

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

agilgur5
Copy link

@agilgur5 agilgur5 commented Jul 22, 2024

Fixes #7921, Fixes #12070, Fixes #12168

Motivation

token not valid errors are very confusing for non-operator end users. they also appear before you've even logged in or after you just logged out.

Modifications

fix(ui): don't show auth error when no cookie

  • per the in-line comment, this generally means you haven't logged in yet

    • unless you're in SSO auth mode, in which case, you would've had an earlier error
    • or in Server auth mode, in which case, you wouldn't get an error in the first place
  • if it's coming from a failed login, instead of saying "Failed to load version/info", say "Login Failed", which is a lot more concrete and understandable at a glance (that doesn't require any knowledge of API endpoints or the code)

  • also refactor some code
    • use getCookie func from the previously unused cookie util file
    • standardize the login page to use the setCookie func as well, which effectively duplicated the logic previously
      • name the login func as setUser, which is a lot less confusing than user
    • also make the "authorization" cookie name a shared constant
    • use async/await for all the auth per UI: Refactor to use async/await syntax #11740

fix(server): be more specific about validation errors

  • "token not valid" is not the most helpful thing, we can say exactly what's wrong with it

    • which we do for SSO auth errors already
  • NOTE: this technically removes the fallback to server auth when using it in conjunction with another one

    • you generally don't want to use server with anything, so that can often be a misconfiguration
      • at my last job, we didn't realize our token was incorrectly formatted until we finally set-up SSO instead of server for the UI (previously we had oauth2-proxy in front)
        • because it was silently falling back to server when client had failed
    • I might change this as it's technically breaking as such, though I think in a good way -- may make that a separate breaking change potentially

Verification

@agilgur5 agilgur5 added this to the v3.5.x patches milestone Jul 22, 2024
Anton Gilgur added 2 commits July 22, 2024 00:37
- per the in-line comment, this generally means you haven't logged in yet
  - unless you're in SSO auth mode, in which case, you would've had an earlier (that the UI may not catch yet, for further commits)
  - or in Server auth mode, in which case, you wouldn't get an error in the first place

- also refactor some code
  - use `getCookie` func from the previously unused `cookie` util file
  - standardize the `login` page to use the `setCookie` func as well, which effectively duplicated the logic previously
    - name the `login` func as `setUser`, which is a lot less confusing than `user`
  - also make the "authorization" cookie name a shared constant
  - use async/await for all the auth

- API changes coming in further commits

Signed-off-by: Anton Gilgur <[email protected]>
- "token not valid" is not the most helpful thing, we can say exactly what's wrong with it
  - which we do for SSO auth errors already

- **NOTE**: this technically removes the fallback to `server` auth when using it in conjunction with another one
  - you generally don't want to use `server` with anything, so that can often be a misconfiguration
    - at my last job, we didn't realize our token was incorrectly formatted until we finally set-up SSO instead of `server` for the UI (previously we had `oauth2-proxy` in front)
      - because it was silently falling back to `server` when `client` had failed
  - I might change this as it's technically breaking as such, though I think in a good way

Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5 agilgur5 force-pushed the fix-server-confusing-login-errors branch from 9c228a5 to d26ee18 Compare July 22, 2024 04:37
- all `document.cookie` manipulation done in this file now
  - move `resetCookie` and consolidate another `getCookie` func

- also use named func instead of var assigned to an anonymous func for better tracing

Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5 agilgur5 force-pushed the fix-server-confusing-login-errors branch from fdacb83 to 5633333 Compare July 22, 2024 04:48
@vlad-ivanov-name
Copy link

hello @agilgur5 ,

do you plan to continue working on this PR? do you need any help?

@Joibel
Copy link
Member

Joibel commented Dec 16, 2024

@vlad-ivanov-name - please feel free to pick this one up and finish it. Please ensure you keep @agilgur5's patches as they are and add on top of them if you're using any of his work, and then the PR get's co-author credits correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants