hawtio / hawtio-online

Hawtio on Kubernetes/OpenShift
Apache License 2.0
23 stars 25 forks source link

HAWNG-557: Stop login page appearing briefly before redirection #364

Closed phantomjinx closed 4 months ago

phantomjinx commented 4 months ago
tadayosi commented 4 months ago

Why I'm concerned with this approach is that it's going to introduce a flow that's not expected in the original design, which is the isRedirecting state. My understanding about the issue is that Hawtio unnecessarily goes into the login page even if it's authenticated under OAuth. If it uses OAuth, authentication should occur outside of the Hawtio page flow, so we should be able to assume that when the flow comes to the Hawtio page it's already authenticated. So we should be able to just let it flow into the main page instead of login. I just wonder why it doesn't work like that.

So, actually this part seems to have a small problem:

https://github.com/hawtio/hawtio-online/blob/cbae6e0da843e8efc0b1466ace47c0f13dfdbb41/packages/oauth/src/openshift/osoauth-service.ts#L308-L311

  1. It returns false even if it explicitly resolves a user (public). The fetchUser API expects it to return true if it's invoked resolve.
  2. It resolves a user even though it doesn't intend to provide it here. What happens if we simply return false here?
phantomjinx commented 4 months ago

Hawtio unnecessarily goes into the login page even if it's authenticated under OAuth

This is unfortunately not the issue. Rather it is that HO displays the login page then redirects when unauthenticated. This does not occur when authenticated.

The isredirecting flag is there to stop any attempt at displaying any react rendering before the js thread then proceeds to leave the page and load the OS-login page. Basically, a mechanism to avoid an invalid state prior to leaving.

Trying to answer your questions above. Am afraid just returning false makes little difference. My machine is normally too quick at redirecting to the OS-login page so I temporarily disabled the redirection and the login page displays (equipped with the wrong title due to the oauth plugin providing the extra login component).

Screenshot_20240307_120817

tadayosi commented 4 months ago

I'm still not convinced that the fix is the right approach. I'm rather wondering if we can fix it by enhancing the page flow of hawtio/react.

phantomjinx commented 4 months ago

I'm still not convinced that the fix is the right approach. I'm rather wondering if we can fix it by enhancing the page flow of hawtio/react.

I know what you mean. The 'add a flag and if-condition' does tend to be a strong sign of a hack. However, am not sure how else to create the break in the JS thread that would allow the redirection to kick-in. My testing seems to indicate that the JS thread wants to end it cycle of code that is in bootstrap.tsx - which ends which an attempted rendering of components.

Maybe it could be as simple as putting that rendering in a timeout to allow the JS thread to 'do something else' for 100ms, which may be enough for the redirection to happen??

tadayosi commented 4 months ago

@phantomjinx @grgrzybek I think I found another solution. Coincidentaly, with hawtio-react 1.1.0 Grzegorz introduced a new state isLoading to User. (Grzegorz, though now the OIDC plugin doesn't rely on the state, right?) Now I understand that this state is needed for handling a case like this issue: when an auth mechanism provides a dedicated login page and Hawtio needs to skip loading the builtin login page.

So, theoretically, whenever it fails to authenticate at the fetchUser hook it should just return:

        resolve({ username: '', isLogin: false, isLoading: true })
        return false

and Hawtio should keep showing the loading view until it's redirected.

I believe it's a more appropriate fix for the issue. Grzegorz, please check if my understanding on the isLoading state is correct.

By the way, if it's the right use case for the isLoading state, it might be that there is a better name for the state. It should rather tell that "the authentication is failed but it provides a dedicated login page so we won't use the default login page".

tadayosi commented 4 months ago

I'm trying to fix the issue in another way #377, but it appears that this issue is also caused by the unnecessary interplay from the hawtio-form-auth plugin; it appears that the form auth plugin still tries to interfere with the auth flow even though the deployment mode is oauth. Generally, the form auth must not impact anything to the auth flow if it's running in a different mode.

tadayosi commented 4 months ago

Let me close it as it's fixed at the other pull req.