Enable OIDC redirect in dashboard #3233

npalaska · 2023-02-06T22:39:47Z

This PR enables the redirect to the OIDC server in the dashboard by using the authentication section of the endpoints API.

It makes the following GET request to the OIDC server:

Auth URI: <auth_url>/realms/<oidc_relam>/protocol/openid-connect/auth

URI params:

client_id=<oidc_client_id>
client_secret=<oidc_client_secret>
response_type=code
redirect_uri=http://<pbench-dashboard>
scope=openid
prompt=login

The request will redirect the user to OIDC broker login page and back to the dashboard page upon successful login

PBENCH-1072

dashboard/src/actions/authActions.js

dbutenhof · 2023-02-07T14:42:56Z

dashboard/src/modules/components/AuthComponent/common-components.jsx

+          <Button
+            variant="primary"
+            onClick={() => {dispatch(authenticationRequest())}}
+          >
+            Red Hat SSO
+          </Button>
          <Button variant="secondary">GitHub</Button>
          <Button variant="secondary">Gmail</Button>


Wasn't the idea that we'd redirect to the Keycloak broker and let it display the menu of SSO options? Are we really skipping a layer here, or redirecting directly to Red Hat?

Yeah, I didn't change the button name but I should update it its not a Red Hat SSO button anymore.

portante

Even though this is dashboard code, I'd like to see this land after PR #3114.

npalaska · 2023-02-07T20:39:13Z

Even though this is dashboard code, I'd like to see this land after PR #3114.

That would make sense because these changes refer to the authentication section of the endpoints API which is going to be renamed as openid-connect, so that would need to be addressed again after #3114 lands.

dashboard/src/modules/components/AuthComponent/common-components.jsx

dashboard/src/actions/authActions.js

webbnh

Part of your change is mutating the Redux state which, as I understand it, we are not supposed to do. Beyond that, I have a few coding suggestions.

dashboard/src/actions/authActions.js

PBENCH-1072

webbnh

Looks better, but the message handling seems like it might want another tweak (and, it would be cool if you fixed the other mutations of the Redux store...).

webbnh · 2023-02-13T22:06:26Z

dashboard/src/actions/authActions.js

+        : { type: TYPES.OPENID_ERROR }
+      );
+      const alert = {
+          title: error?.response ? error.response.data?.message : error?.message,


This feels funny: it says "if there is an error.response then there must be an error.response.data which might contain a message which we can use for the title (but, if it doesn't, then use a title of undefined); otherwise, use a title of error.message (or maybe undefined). That's a lot of undefined's....

In any case, it seems like what we want is something more like this:

title: error.response?.data?.message ? error.response.data.message : error?.message ? error.message : "Unexpected error",

That is, if our error.response contains a data which contains a message, then use that message; otherwise, if our error contains a message then use that message; otherwise, fall back to a constant message.

(The current code is assuming that error.response contains a data which may or may not contain a message...are we sure it contains a data?...are we sure that data might not contain a message?)

Okay, this is a good suggestion but I will address it in #3250, I need to do some rework there anyway.

dbutenhof

It'd be good to take Webb's error message suggestion, although perhaps that can be done in the follow-on.

npalaska · 2023-02-23T21:24:21Z

Hi @portante can you consider removing the objection or reviewing it?

Not reviewing code on the openid-connect branch.

Enable OIDC redirect in the dashboard PBENCH-1072

npalaska self-assigned this Feb 6, 2023

npalaska added Dashboard Of and relating to the Dashboard GUI Users Of and relating to working with users. labels Feb 6, 2023

npalaska added this to the v0.72 milestone Feb 6, 2023

npalaska requested review from MVarshini, dbutenhof, webbnh and portante February 6, 2023 22:40

portante requested changes Feb 7, 2023

View reviewed changes

dashboard/src/actions/authActions.js Outdated Show resolved Hide resolved

dbutenhof previously approved these changes Feb 7, 2023

View reviewed changes

npalaska dismissed dbutenhof’s stale review via aebd2c8 February 7, 2023 20:02

npalaska requested review from portante and dbutenhof February 7, 2023 20:03

npalaska force-pushed the pbench_1072 branch from aebd2c8 to 63b3a25 Compare February 7, 2023 20:03

portante requested changes Feb 7, 2023

View reviewed changes

dbutenhof previously approved these changes Feb 7, 2023

View reviewed changes

dashboard/src/modules/components/AuthComponent/common-components.jsx Show resolved Hide resolved

npalaska dismissed dbutenhof’s stale review via 9dddc36 February 8, 2023 01:17

npalaska force-pushed the pbench_1072 branch from 9dddc36 to 35a98fc Compare February 8, 2023 01:17

npalaska requested review from portante and dbutenhof February 8, 2023 01:18

portante previously requested changes Feb 8, 2023

View reviewed changes

dashboard/src/actions/authActions.js Outdated Show resolved Hide resolved

dbutenhof previously approved these changes Feb 8, 2023

View reviewed changes

npalaska dismissed dbutenhof’s stale review via 8c7cfe4 February 8, 2023 20:13

npalaska requested review from portante and dbutenhof February 8, 2023 20:13

webbnh requested changes Feb 8, 2023

View reviewed changes

npalaska mentioned this pull request Feb 9, 2023

Renameopenid-connect variable in the endpoints api to openid or openid_connect? #3240

Closed

npalaska mentioned this pull request Feb 9, 2023

Remove hyphen from openid-connect section of the endpoints api #3241

Merged

npalaska changed the base branch from main to openid-connect February 9, 2023 18:47

npalaska added 5 commits February 11, 2023 11:43

Enable OIDC redirect in dashboard

31de196

PBENCH-1072

update the buttons on UI

db6499c

add try-catch, sync with latest server endpoints api changes

00d41a9

we dont need client secret when redirecting

d32e20b

refactor and remove redux state mutation

de04230

npalaska force-pushed the pbench_1072 branch from 8c7cfe4 to de04230 Compare February 11, 2023 17:17

npalaska mentioned this pull request Feb 12, 2023

Get OIDC access tokens once the authentication redirect is successful #3250

Merged

npalaska requested a review from webbnh February 13, 2023 14:18

webbnh approved these changes Feb 13, 2023

View reviewed changes

dbutenhof approved these changes Feb 17, 2023

View reviewed changes

Merge branch 'openid-connect' into pbench_1072

852762e

npalaska merged commit e484a38 into distributed-system-analysis:openid-connect Feb 25, 2023

npalaska added a commit to npalaska/pbench that referenced this pull request Mar 9, 2023

Enable OIDC redirect in dashboard (distributed-system-analysis#3233)

f4161fa

Enable OIDC redirect in the dashboard PBENCH-1072

npalaska added a commit that referenced this pull request Mar 13, 2023

Enable OIDC redirect in dashboard (#3233)

d3e939c

Enable OIDC redirect in the dashboard PBENCH-1072

npalaska added a commit to npalaska/pbench that referenced this pull request Mar 27, 2023

Enable OIDC redirect in dashboard (distributed-system-analysis#3233)

4ad67a1

Enable OIDC redirect in the dashboard PBENCH-1072

npalaska added a commit to npalaska/pbench that referenced this pull request Mar 27, 2023

Enable OIDC redirect in dashboard (distributed-system-analysis#3233)

f2aa956

Enable OIDC redirect in the dashboard PBENCH-1072

npalaska added a commit to npalaska/pbench that referenced this pull request Mar 28, 2023

Enable OIDC redirect in dashboard (distributed-system-analysis#3233)

723addc

Enable OIDC redirect in the dashboard PBENCH-1072

npalaska added a commit to npalaska/pbench that referenced this pull request Mar 30, 2023

Enable OIDC redirect in dashboard (distributed-system-analysis#3233)

00865f0

Enable OIDC redirect in the dashboard PBENCH-1072

npalaska added a commit to npalaska/pbench that referenced this pull request Mar 31, 2023

Enable OIDC redirect in dashboard (distributed-system-analysis#3233)

97b92e2

Enable OIDC redirect in the dashboard PBENCH-1072

npalaska mentioned this pull request Mar 31, 2023

Openid connect -> main #3353

Merged

npalaska added a commit that referenced this pull request Mar 31, 2023

Enable OIDC redirect in dashboard (#3233)

06b0718

Enable OIDC redirect in the dashboard PBENCH-1072

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable OIDC redirect in dashboard #3233

Enable OIDC redirect in dashboard #3233

npalaska commented Feb 6, 2023 •

edited

Loading

dbutenhof Feb 7, 2023

npalaska Feb 7, 2023

portante left a comment

npalaska commented Feb 7, 2023

webbnh left a comment

webbnh left a comment •

edited

Loading

webbnh Feb 13, 2023 •

edited

Loading

npalaska Feb 23, 2023

dbutenhof left a comment

npalaska commented Feb 23, 2023

Enable OIDC redirect in dashboard #3233

Enable OIDC redirect in dashboard #3233

Conversation

npalaska commented Feb 6, 2023 • edited Loading

dbutenhof Feb 7, 2023

Choose a reason for hiding this comment

npalaska Feb 7, 2023

Choose a reason for hiding this comment

portante left a comment

Choose a reason for hiding this comment

npalaska commented Feb 7, 2023

webbnh left a comment

Choose a reason for hiding this comment

webbnh left a comment • edited Loading

Choose a reason for hiding this comment

webbnh Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

npalaska Feb 23, 2023

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

npalaska commented Feb 23, 2023

npalaska commented Feb 6, 2023 •

edited

Loading

webbnh left a comment •

edited

Loading

webbnh Feb 13, 2023 •

edited

Loading