AxaFrance / oidc-client

Light, Secure, Pure Javascript OIDC (Open ID Connect) Client. We provide also a REACT wrapper (compatible NextJS, etc.).
MIT License
570 stars 152 forks source link

Unable to check for logged in user by redirection #1375

Open lmm-git opened 1 month ago

lmm-git commented 1 month ago

Issue and Steps to Reproduce

I am using oidc-client (not the react variant).

Some IdPs like Keycloak support authentication with property `prompt=none. This causes the IdP typically to redirect a user directly back to the application if there is no active session without prompting the user to authenticate. This is also how abort buttons on some IdPs are implemented.

In such case, the IdP is redirecting back to the application with an error and error_description. However, currently the library just raises a generic Exception in this case: https://github.com/AxaFrance/oidc-client/blob/630b4faed4ff275712f2f2ae00be64f8587cfc4b/packages/oidc-client/src/login.ts#L116

This is a problem in multiple instances:

  1. An application has a hard time detecting such error (basically only string matching is left)
  2. An application will not receive the previously store redirect URL, causing the user to be stuck on the callback URL

Both problem could be solved by

  1. raising a proper Exception which includes those values as fields or
  2. by providing the normal LoginCallback object with additional details (like whether the login was successful etc.).

Which option do you think is the best one? Or does the library already support this use case and I just overlooked it?

Versions

@axa-fr/oidc-client 7.22.6

Additional Details

One could circumvent this issue by reading the redirection URL before calling loginCallbackAsync via

import { initSession } from '@axa-fr/oidc-client/src/initSession'

const session = initSession('default', this.oidcClient.configuration.storage)

// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
const redirectUrl: string = session.getLoginParams().callbackPath

and detecting the error like

try {
  const loginCallback = await this.oidcClient.loginCallbackAsync()
  if (loginCallback.callbackPath !== undefined) {
    // redirect user back to original URL
    await this.router.replace(loginCallback.callbackPath)
  }
} catch (err) {
  const castErr: Error = err as Error
  // handle IdP login_required error
  if (castErr.message.includes('login_required')) {
    this.loadingState = UserLoadingState.UNAUTHENTICATED
    // redirect user back to original URL
    await this.router.replace(redirectUrl)
  } else {
    // just re-throw the error
    throw err
  }
}

But this is really hacky. Additionally, the initSession script is not intended to be used directly I think.

guillaume-chervet commented 1 month ago

Hi @lmm-git , I really prefer option 2 and code everything with data information. I'am lacking of time so i code first iteration with ugly exception :/

Do you need it quickly? I think i will be pretty busy during june.

lmm-git commented 1 month ago

Hi, thanks for your quick response!

for now, I can make use of my little hack. If it gets fixed some time in future it is fine for me as I just don't want to use a library which does not want to support this use case. But this doesn't seem to be the case here ^^

Personally, I like option 2 more as well, but this is definitely a breaking change if you don't want to create a new function for it.