Get OIDC access tokens once the authentication redirect is successful

[npalaska]

On the Pbench Dashboard when the OIDC authentication redirect request is successful, the OIDC broker will redirect the user back to the Pbench auth page with a location header containing session_id and code. We have to use this code to make a POST request against the OIDC token endpoint. If the token endpoint request is successful we log the user in and redirect the user to the overview page.

The POST request might look like the following:

http://<auth-server>/realms/pbench/protocol/openid-connect/token

grant_type=authorization_code
code=<code>
client_id=pbench-client
redirect_uri=http://<pbench-dashboard>/login

Note: This also makes a request to the OIDC userinfo endpoint to fetch and then store the user info as a state value.

This PR is currently rebased on commits from PR #3233

PBENCH-1073

[webbnh]

Here are a few things to consider.

[dbutenhof]

Also ... you should look in src/utils/axiosInstance.js. We have an API interceptor that adds the bearer token and reports authentication failures. That code ought to be handling expiration by attempting to use the refresh token to negotiate a new access token; otherwise our joy in using the new dashboard code will be shortlived. 😆

(And I appear to have a mind comment pending that I "abandoned" days ago without submitting. Nice of GitHub to remember it, I guess...)

[npalaska]

Okay, opened for review now and I have tried to address most of the relevant comments, but many here may no longer be relevant.

[webbnh]

There is a bunch more code to be ripped out. 🙂

[dbutenhof]

Coming along nicely. As many of Webb's comments suggest, a Prettier run would be good. We should probably add the prettier hook into our eslint config to check formatting in the CI as we do for Python, which would eliminate most concerns of that nature during reviews.

[dbutenhof]

Yeah, I almost kinda wish the prettierization had been a separate PR. It was a bit awkward to re-review this morning because, while the reformatting was segregated in a separate commit (thanks), there were new code changes on top of that commit. Ah, well; water over and/or under the bridge ...

Anyway, I think we're converging here. I'm still not sure I understand the flows around page reload and the interactions with cookies vs redux store as related to the OIDC session management, and a bit more clarity would help. Beyond that I think we're into streamlining and cleanup...

[webbnh]

There is still a bunch more dead code to be removed, and I think you should take a close look at all the (new) places where you are using ?. (I pointed out a bunch of suspicious ones). And, there are a few other nits and small things.

[webbnh]

BTW, thanks for putting the Prettier changes in their own commit! FWIW, when Dave and I suggested running Prettier, I think we were thinking that it would affect only your changes or only your changed modules. (We weren't really aware that so much of the overall code was not up to spec....)

Below is an additional question I had. Also, there are a few lingering items from my previous review-passes:

• The location of the endpoint-load timeout length definition.
• My request for a value instead of a code block in an arrow function value.
• The question of logout vs. session expiration handling (although, the difference has gotten pretty small, now).
• The question about referring unlogged-in users to OIDC (although, perhaps this will be moot with RH SSO, or it will be obvious from context and won't need to be explicit).
• And, the question of how the user's session is supposed to survive browser reloads.

[webbnh]

This if statement has no else block...which is odd....

What happens if the user fails his/her authentication? Are they "stuck" on the OIDC login page and never come back to the Dashboard? Or, does OIDC somehow send them back with failure status?

That is, when execution reaches line 40 here, can authenticated ever be anything but true? If so, shouldn't we have an else branch which acknowledges that the login failed (i.e, in opposition to line 44)? And, if not, do we really need/want to be testing it?

[npalaska]

What happens if the user fails his/her authentication? Are they "stuck" on the OIDC login page and never come back to the Dashboard? Or, does OIDC somehow send them back with failure status?

Yeah, the Keycloak will not redirect them back to the dashboard in case of authentication failure, they will be given a chance to re-enter their credentials. Users are not necessarily stuck there they can hit the back button of the browser to come back to the dashboard landing page and have a view of the non-logged-in user.

That is, when execution reaches line 40 here, can authenticated ever be anything but true? If so, shouldn't we have an else branch which acknowledges that the login failed (i.e, in opposition to line 44)? And, if not, do we really need/want to be testing it?

We could display toastAction like else {dispatch(showToast("danger", "Not authenticated!"));} but I don't think that's necessary. Landing on auth page without logging in, users already know that they are not logged in and we don't need to explicitly tell them that.

[webbnh]

I guess I'm confused about the sequencing.

When the Dashboard is initially loaded, we arrive at the code here before the user does the Keycloak dance, but after we've initialized the Keycloak plug-in and created the keycloak object. So, on the first pass through here, keycloak.authenticated will be false, and that's OK, and we'll go on to render some sort of helpful default page. On subsequent page renderings (e.g., when the Keycloak login redirects back to the Dashboard), we'll come through here and keycloak.authenticated will be true, and so we'll set our cookie-thing here and raise a toast to the user's success.

So, to answer my own questions: yes, we do want to test keycloak.authenticated here (because initially, at least, it will be false); no, we don't need an else clause on this if (because that execution path is "normal" and unremarkable); and, as you said, when an authentication attempt fails, we won't end up here.

But, execution will come through here at other times, right? We don't necessarily want to toast the user every time, do we? (We may or may not want to update the cookie...I'm less clear on that...but, then, I'm not sure that we need/want this cookie at all....)

So, I'm still wondering about this if statement: perhaps it should be moved to somewhere where we know the answer and don't have to test for it.

[npalaska]

I'm not sure that we need/want this cookie at all....

isLoggedIn cookie has an expiry of refreshToken so it lives across the multiple App rendering. I could check the value of keycloak.authenticated inside the protected route of the App but on each rendering, the keycloak object gets reinitialized and so the value is not there yet. The problem is if I don't use this cookie in the protected route and instead rely on keycloak.authenticated value when the App renders it's gonna immediately display the DANGER toast and redirect the auth page.

I also think ideally we shouldn't need to maintain cookies internally related to the authentication but I couldn't find a way around it at this time. This is something I need to spend some more time understanding the keycloak-js plugin and fix this reload problem.

So, I'm still wondering about this if statement: perhaps it should be moved to somewhere where we know the answer and don't have to test for it.

What do you mean? We know the answer to what happens on the if statement and what will happen outside the if statement, right?

[webbnh]

What do you mean?

We need the if statement here (we think...) because, at this point in the code, we don't know whether keycloak.authenticated is true or false. The question is, is there some point, up- or down-stream from here, where we already know the answer? If so, perhaps we can move the if statement there, so that we don't have to ask the question here (or there!).

[dbutenhof]

Wow: that's a really weird unit test failure, considering you haven't changed any server code. Hard to imagine re-submitting will make any difference, but I'm trying anyway.

[npalaska]

Wow: that's a really weird unit test failure, considering you haven't changed any server code. Hard to imagine re-submitting will make any difference, but I'm trying anyway.

I guess re-submitting worked mysteriously 🤔

[webbnh]

that's a really weird unit test failure, considering you haven't changed any server code

I'm guessing that it's a missing fixture (or a fixture scope change, or a change to a "chain" of fixture inclusions) in the test code, and that it has little or nothing to do with Server code.

[webbnh]

I guess re-submitting worked mysteriously

Apparently so. 😬

[webbnh]

These latest changes look good; we just need to conclude the discussions from the previous reviews.

[webbnh]

Looking good. I think you should add some more to the getDatasets error handler. Beyond that, I found what I think are a couple of unneeded async's, a handful of unneeded ?.'s, and some other small things which you should consider addressing.

And, there are a few lingering issues from my earlier passes:

• Account creation date (can't we just remove the whole ProfileComponent?)
• The untested ?. result in the HeaderComponent.
• The if statement in authCookies().

But, I think we're getting close.

[webbnh]

This looks like an orphan which should have been removed. (I.e., I think this was a placeholder for the second half of a ternary; you removed the first half, so now we don't need the "no-op" second half.)

[MVarshini]

The less files formatting has been changed.

Also, imports in few files are changed from " " to ' '

[npalaska]

Also, imports in few files are changed from " " to ' '

I just re-checked, prettier took care of it and it seems like all the imports in all the files now have " "

The less files formatting has been changed.

Are we not supposed to change less files. prettier actually re-formatted them.

[webbnh]

This looks good enough for now.

I think there is an orphan bit of code which should have been removed, but it's the HTML/Javascript equivalent of a no-op, so it's not worth haggling over.

And, I'm still unsettled by the if statement in authCookies(), but we can work on "flow" later.

[dbutenhof]

I'm not thrilled about the error formatting, but this is a problem throughout the dashboard we probably need to address separately.

[dbutenhof]

How will Javascript format a Response object? Is that really what we want to appear in the toast? If the response body is a JSON and has message we want to show it; it could also be JSON without a message, or a non-JSON response payload, or there could be no payload at all. I'm not sure how "clever" we want to be here, but probably the final fallback would be the HTTP status code and the code's generic message (response.reason, I think, in Python, though I'm not sure offhand how the Javascript response object differs.) So, maybe, just error.response.data.message or the HTTP status and reason? (But what if we get an error exception that doesn't even have a Response object? In this code we're back to a stringified undefined.)

[webbnh]

How will Javascript format a Response object?

It's a valid question which I decided to punt on the principal of the perfect being the enemy of the done. This is something that we can iterate on.

Is that really what we want to appear in the toast?

Probably, not; but my instinct was that it was better than a generic (local fixed-text) message.

I'm not sure how "clever" we want to be here

Only as clever as we need to be and no cleverer. If you're reasonably sure that there will be a response.reason, I'm OK with using that instead of the whole response (I was hoping that Javascript would do something reasonable, and that having the whole response might be be illuminating). If we use the chaining dereference, I'm OK with seeing an undefined in the rare case that we get an error with no other information (i.e., that's not much different from getting a literal "something went wrong" message....)