Openid connect -> main #3353

npalaska · 2023-03-17T18:51:50Z

Since we have now cut a deployment branch that's 0.72, its time to bring the changes from the openid-connect branch on the main and sync with the main.

dbutenhof

I wonder if you should hold off here until you've got the dashboard Keycloak package and hooks integrated? This will break the dashboard until that's done. But maybe that's OK ... ❔ 👀

In any case, there seem to be a few empty files, and a few other possible problems ...

lib/pbench/server/api/resources/__init__.py

lib/pbench/server/api/resources/users_api.py

lib/pbench/server/database/models/active_tokens.py

lib/pbench/test/functional/server/test_put.py

lib/pbench/test/functional/server/conftest.py

webbnh

Here's what I've got so far...have a good weekend!

lib/pbench/cli/server/user_management.py

lib/pbench/client/__init__.py

lib/pbench/client/oidc_admin.py

webbnh

Here's the next installment of feedback. I'll look at the Dashboard changes tomorrow.

lib/pbench/server/api/resources/__init__.py

lib/pbench/server/api/resources/datasets_metadata.py

server/pbenchinacan/load_keycloak.sh

server/lib/config/pbench-server-default.cfg

lib/pbench/test/unit/server/database/test_users_db.py

lib/pbench/test/unit/server/query_apis/test_datasets_detail.py

lib/pbench/test/unit/server/test_schema.py

lib/pbench/test/unit/server/test_shell_cli.py

lib/pbench/test/unit/server/test_user_auth.py

webbnh

OK, the first pass is done. Bring on the updates!

dashboard/src/actions/authActions.js

webbnh

Coming along nicely.

The new parameter for the _decode() function looks like it shouldn't be optional/None, and it should be specified in the new calls.

The name of the ODIC client in the Pbench realm should be something more generic than pbench-dashboard...unless we're planning on configuring custom clients for all of our programmatic users....

Other than those, my new comments are just nits and small stuff.

There are a couple of comments from my previous reviews which you missed or need attention:

Docstring update.
Dashboard error handling.
The User table delete function.
The "username" column in the Alembic migration code.

lib/pbench/server/database/models/users.py

lib/pbench/test/unit/server/auth/test_auth.py

lib/pbench/test/unit/server/test_authorization.py

lib/pbench/test/unit/server/test_datasets_inventory.py

server/pbenchinacan/load_keycloak.sh

dbutenhof

Since this appears to be still in draft, I just made a scan to follow up on my previous comments, most of which (save one) seem to be resolved...

lib/pbench/test/functional/server/test_put.py

dbutenhof

Other than the dangling endpoints client reference that blew the tests, this is looking good.

lib/pbench/test/unit/server/database/test_datasets_db.py

…ibuted-system-analysis#3241) Remove hyphen from openid-connect section of the endpoints API - Getting rid of the excess quotes and brackets would make this easier to read - It also makes it easier to reference in Dashboard javascript code when there is no hyphen involved (endpoints.openid instead of endpoints['openid-connect'])

…m-analysis#3239) * Remove unused userinfo and online token validation - Removes the get_userinfo functionality from the auth module - Removes the online token validation capability from the server - OIDC token validation will only happen locally PBENCH-1074

…tem-analysis#3243) Update the keycloak.sh to not create a private client. Instead of creating a private pbench-server-client, it now creates pbench-dashboard public client.

…uted-system-analysis#3249) Use of OIDC configuration in the server unit test as default. We have decided to keep the internal Users table for the purpose of quick user lookup and logging. However, unlike the current Users table, there won't be any user credentials stored in the internal database. To facilitate this transition we have to update the User table and active tokens table. This will break all the unit tests because of the way they are set up right now. Currently, we update the active tokens table when we create a new JWT token in the unit test and update the respective user table relationship. And validation of the token depends on it being available in the active tokens table. This change is to use OIDC configuration in the unit tests and start using RS256 to encode and decode the JWT tokens (using OIDC token introspection). And remove the active tokens dependency from the unit tests. PBENCH-1079

* Use OIDC user for the functional test Functional tests should move to using OIDC tokens by making an Admin REST API request to the OIDC server (Keycloak) directly as the server won't be providing Register/Login capabilities on its endpoints. - Create a new scope in the Keycloak to add OIDC client name in aud claim (required for the authentication) - Functional test user registration happens directly with the OIDC server using the Admin token - Functional test user makes a REST API call to get the OIDC token PBENCH-1070

Enable OIDC redirect in the dashboard PBENCH-1072

* Rework the User table Rework involves the following things: 1. Modifications to the User table: - Drop the password column - Drop the auth_tokens relationship (will be later replaced with API keys table) - Add a column to keep track of OIDC_IDs of the user - Add the user relationship to the Datasets and Metadata table 2. Removed the user API 3. Changes to the user_management_cli file: - Kept only list users functionality. PBENCH-1080

…distributed-system-analysis#3250) * Dashboard integration with keycloak - New dependency added @react-keycloak/web and keycloak-js - Checks the SSO session by wrapping the entire App inside KeycloakProvider. - Removed the current use of login and registration related components. - Removed all the changes and files that got void after this authentication change. - Also ran prettier on the entire src directory. PBENCH-1073

dbutenhof

I guess if Mr Jenkins is happy, I'm happy.

webbnh · 2023-03-31T19:29:26Z

dashboard/src/actions/overviewActions.js

-      dispatch(
-        showToast(DANGER, msg ? msg : `Error response: ERROR_MSG`)
-      );
+      dispatch(showToast(DANGER, msg ? msg : `Error response: ERROR_MSG`));


No need to change this unless you have other changes to make, but a recent change from Varshini brought to light the fact that we have alternatives to the use of the ternary operator, here. For instance, either || or ?? might be nicer.

dispatch(showToast(DANGER, msg || `Error response: ERROR_MSG`));

(?? will use the default message if msg is undefined; || will use the default message if msg is an empty string; but using either of them allows us to avoid the second reference to msg...just some more items for our Javascript toolboxes....)

Okay, I don't have any blocking changes outstanding in this PR now so I'll defer it in another PR.

lib/pbench/server/database/alembic/versions/f628657bed56_user_table_update_oidc.py

webbnh

This looks good to go.

I found two more nits after the earlier one that GitHub helpfully posted before I I was ready, and there's one more which I think I called out before but cannot find the comment for...but none of these need to gate the merge of this PR.

webbnh · 2023-03-31T19:42:39Z

lib/pbench/test/unit/server/database/__init__.py

@@ -312,6 +312,10 @@ def commit(self):
        property has been set, "fail" by raising the exception. Otherwise,
        mock a commit by updating any cached "committed" values if the "known"
        proxy objects have changed, and record any new "added" objects.
+
+        Commit operation now also support delete, however, note that deletes


You're missing an s, somewhere in that line. 😉

Sigh! I'll fix it in the next PR.

webbnh · 2023-03-31T19:59:56Z

dashboard/src/actions/authActions.js

+export const sessionLogout = () => async (dispatch, getState) => {
+  const keycloak = getState().apiEndpoint.keycloak;
+  keycloak.logout();
+  clearCachedSession(dispatch);
 };


In general, I don't think that there is a requirement for a thunk to be an async function (although, if the function is not async, then there's a question as to why it needs to be called via dispatch() instead of being invoked directly, but that's neither here nor there...).

Beyond that, I don't see anything in the body of this function which requires the function to be declared async.

So, I think (if, e.g., you needed to make an edit for some other reason), you can drop the async from here.

Yeah in this PR I just sort of followed how the functions were written already in this file without giving a lot of thought to javascript async operations (which I now have a better understanding of than before I started working on #3233 and #3250). But if this is not a blocking change then I'll defer to my next dashboard PR (Removal of the user profile page).

webbnh

No visible changes 🙈, it must be good. 👍

npalaska changed the title ~~Openid connect~~ Openid connect -> main Mar 17, 2023

dbutenhof reviewed Mar 17, 2023

View reviewed changes

webbnh reviewed Mar 17, 2023

View reviewed changes

webbnh reviewed Mar 21, 2023

View reviewed changes

webbnh requested changes Mar 21, 2023

View reviewed changes

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

npalaska force-pushed the openid-connect branch from 2d0b2f8 to 508f3a4 Compare March 27, 2023 04:02

webbnh reviewed Mar 27, 2023

View reviewed changes

npalaska force-pushed the openid-connect branch from 508f3a4 to c662a02 Compare March 27, 2023 22:53

dbutenhof reviewed Mar 28, 2023

View reviewed changes

lib/pbench/test/functional/server/test_put.py Outdated Show resolved Hide resolved

npalaska force-pushed the openid-connect branch 2 times, most recently from e19fd2c to 1866d5f Compare March 30, 2023 13:35

npalaska marked this pull request as ready for review March 30, 2023 13:36

npalaska requested review from webbnh and dbutenhof March 30, 2023 13:36

dbutenhof reviewed Mar 30, 2023

View reviewed changes

dbutenhof previously approved these changes Mar 30, 2023

View reviewed changes

lib/pbench/test/unit/server/database/test_datasets_db.py Show resolved Hide resolved

npalaska added 8 commits March 31, 2023 11:54

Make the OAuth2 client public in our Keycloak config (distributed-sys…

5c7caf1

…tem-analysis#3243) Update the keycloak.sh to not create a private client. Instead of creating a private pbench-server-client, it now creates pbench-dashboard public client.

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

97b92e2

Enable OIDC redirect in the dashboard PBENCH-1072

npalaska dismissed dbutenhof’s stale review via be35713 March 31, 2023 16:14

npalaska force-pushed the openid-connect branch from a993450 to be35713 Compare March 31, 2023 16:14

npalaska requested a review from dbutenhof March 31, 2023 16:16

dbutenhof previously approved these changes Mar 31, 2023

View reviewed changes

webbnh reviewed Mar 31, 2023

View reviewed changes

webbnh previously approved these changes Mar 31, 2023

View reviewed changes

Sync openid-connect branch with main

6374bdd

npalaska dismissed stale reviews from webbnh and dbutenhof via 6374bdd March 31, 2023 20:52

npalaska force-pushed the openid-connect branch from be35713 to 6374bdd Compare March 31, 2023 20:52

npalaska requested review from webbnh and dbutenhof March 31, 2023 20:54

webbnh approved these changes Mar 31, 2023

View reviewed changes

npalaska merged commit 8d54f43 into distributed-system-analysis:main Mar 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Openid connect -> main #3353

Openid connect -> main #3353

npalaska commented Mar 17, 2023

dbutenhof left a comment

webbnh left a comment

webbnh left a comment

webbnh left a comment

webbnh left a comment •

edited

Loading

dbutenhof left a comment

dbutenhof left a comment

dbutenhof left a comment

webbnh Mar 31, 2023

npalaska Mar 31, 2023

webbnh left a comment

webbnh Mar 31, 2023

npalaska Mar 31, 2023

webbnh Mar 31, 2023

npalaska Mar 31, 2023

webbnh left a comment

Openid connect -> main #3353

Openid connect -> main #3353

Conversation

npalaska commented Mar 17, 2023

dbutenhof left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

webbnh left a comment • edited Loading

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

webbnh Mar 31, 2023

Choose a reason for hiding this comment

npalaska Mar 31, 2023

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

webbnh Mar 31, 2023

Choose a reason for hiding this comment

npalaska Mar 31, 2023

Choose a reason for hiding this comment

webbnh Mar 31, 2023

Choose a reason for hiding this comment

npalaska Mar 31, 2023

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

webbnh left a comment •

edited

Loading