Add OIDC user to the functional test #3235

npalaska · 2023-02-07T04:42:23Z

PBENCH-1070

To facilitate the removal of the dependency on the internal user API and internal users table we need to update the functional tests to not use the internal APIs.

Functional tests should move to using OIDC tokens by making a Admin REST API request to the OIDC server (Keycloak) directly as the server won't be providing those capabilities on its endpoints.

This PR is currently rebased on top of PR #3114

npalaska · 2023-02-08T17:03:03Z

Note:

These changes introduce the OIDC admin functionality for the client to use where the user directly communicates with the OIDC server and gets the token and the Dashboard is not involved in this handshake.
These changes are only related to functional tests and do not update the unit tests to use the OIDC-compliant tokens and users. With PR Further cleanup for the auth module #3114 we segregated the operational Pbench-server where we can either deploy the server with OpenID or the with an internal tokens table. In this effort, we have reverted unit tests to use the internal tokens, I'll fix this in another PR that handles only unit tests changes.

portante · 2023-02-08T20:13:26Z

lib/pbench/client/oidc_admin.py

+from requests.structures import CaseInsensitiveDict
+
+
+class OIDCAdmin:


We have the Connection class in the lib/pbench/server/auth/__init__.py module which we should probably re-use.

I'm going to defer reviewing this code until this change is made.

portante

We should not re-add the same code that is already in the Connection class.

portante · 2023-02-08T20:14:32Z

lib/pbench/server/auth/__init__.py

@@ -468,7 +468,7 @@ def token_introspect_offline(self, token: str) -> JSON:
                token,
                self._pem_public_key,
                algorithms=[self._TOKEN_ALG],
-                audience=self.client_id,
+                audience=["account", self.client_id],


Where is this audience coming from?

Oops, I forgot to add a comment here.
Basically, if a user logs in with a username and password directly with a Keycloak rest API using private client then the aud claim included in the token is account
for example this request:

requests.post( "<token_endpoint>", headers=<{headers}>, data={"client_id": "pbench-server-client", "client_secret": <secret>, "scope": "profile email", "grant_type": "password", "username": user, "password": "123"}, verify=False )

will generate a token that has a payload like following:

{'exp': 1675890382, 'iat': 1675890082, 'jti': 'a8dc9cca-d734-4758-ab1d-be6510fe85b2', 'iss': 'http://localhost:8090/realms/pbench-server', 'aud': 'account', 'sub': '9202c4a9-5901-46dc-9b8e-2643a5f4372e', 'typ': 'Bearer', 'azp': 'pbench-server-client', 'session_state': '5730b4b1-bdb3-438c-a308-d3c1f8108fd8', ....}

And right now our functional test user is logging in using the Keycloak Rest API, so I had to include this claim to get the functional tests to pass.

This one should be fixed now. I have added a client scope in the Keycloak configuration that will add the Pbench openid client (pbench-dashboard) in the aud claim by default. So the functional test user tokens should be able to get validated now.

@portante can we resolve this comment?

We don't require comments to be resolved before merging. Forgive me, but I have not had the time to review the changes in detail.

In that case, can you consider removing your objection to this PR, its blocking the merge.

dbutenhof

I don't think I got finished; but I just realized I left comments so I might as well publish them before today ends.

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

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

webbnh

Based on the dependencies of this PR, should it be moved to draft mode?

webbnh · 2023-02-09T16:37:22Z

lib/pbench/client/oidc_admin.py

+from requests.structures import CaseInsensitiveDict
+
+
+class OIDCAdmin:


I'm going to defer reviewing this code until this change is made.

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

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

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

npalaska · 2023-02-09T18:09:33Z

Converted this to the draft mode again, I need to work on the new users table that we talked about in the design sync meeting.

portante · 2023-02-09T18:12:54Z

Converted this to the draft mode again, I need to work on the new users table that we talked about in the design sync meeting.

Please don't create a new users table. We already have one.

npalaska · 2023-02-09T18:56:48Z

Converted this to the draft mode again, I need to work on the new users table that we talked about in the design sync meeting.

Please don't create a new users table. We already have one.

Well, we can't use the current users table, it has user credentials information in it. So we have to drop it entirely and create a have a new one without all the user login stuff.

dbutenhof · 2023-02-09T19:04:10Z

Converted this to the draft mode again, I need to work on the new users table that we talked about in the design sync meeting.

Please don't create a new users table. We already have one.

Well, we can't use the current users table, it has user credentials information in it. So we have to drop it entirely and create a have a new one without all the user login stuff.

That's a semantic distinction I wouldn't worry about. You probably want to drop most of the existing columns. We only need a uuid = Column(String, unique=True) and a username = Column(String)... The new "API key" (similarly reworked/simplified AuthToken table will want a foreign key link, so probably also the same old id = Column(Integer, primary_key=True, autoincrement=True). We might want to capture Pbench-specific "profile" stuff here, and one way would be a simple profile = Column(JSON) where we can store anything we want.

portante

Can you rebase this on the openid-connect branch?

npalaska · 2023-02-10T18:52:00Z

I changed the base but I'll rebase it on the openid-connect in some time.

webbnh

Now that you've added support for the json parameter, you should revert the changes to add the str type to the data parameter (I'm hoping there's no reason why you cannot do that...). And, there are a bunch of nits which you should fix. And, I've got some suggestions, questions, and the odd request.

lib/pbench/client/__init__.py

lib/pbench/server/auth/__init__.py

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

lib/pbench/client/oidc_admin.py

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

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

lib/pbench/server/auth/__init__.py

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

webbnh

Looks great, Nikhil.

Just a couple of tiny nits if you are so inclined.

lib/pbench/client/oidc_admin.py

lib/pbench/server/auth/__init__.py

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

lib/pbench/server/auth/__init__.py

webbnh

Ship it!

lib/pbench/server/auth/__init__.py

No longer participating in the review cycle for this PR.

* 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

npalaska self-assigned this Feb 7, 2023

npalaska added the Functional Testing label Feb 7, 2023

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

dbutenhof mentioned this pull request Feb 7, 2023

Further cleanup for the auth module #3114

Merged

npalaska force-pushed the pbench_1070 branch 2 times, most recently from ae1a077 to 73ac125 Compare February 8, 2023 16:50

npalaska marked this pull request as ready for review February 8, 2023 16:50

npalaska requested review from webbnh, dbutenhof, portante, MVarshini and ndokos February 8, 2023 16:50

portante reviewed Feb 8, 2023

View reviewed changes

portante requested changes Feb 8, 2023

View reviewed changes

dbutenhof reviewed Feb 8, 2023

View reviewed changes

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

lib/pbench/server/api/resources/__init__.py Show resolved Hide resolved

webbnh requested changes Feb 9, 2023

View reviewed changes

dbutenhof requested changes Feb 9, 2023

View reviewed changes

lib/pbench/server/api/resources/__init__.py Show resolved Hide resolved

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

npalaska marked this pull request as draft February 9, 2023 18:08

portante previously requested changes Feb 10, 2023

View reviewed changes

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

npalaska force-pushed the pbench_1070 branch from 73ac125 to f100592 Compare February 13, 2023 03:38

npalaska mentioned this pull request Feb 13, 2023

Rework the User table #3251

Merged

npalaska force-pushed the pbench_1070 branch from 9f96d38 to c0c8625 Compare February 15, 2023 00:46

npalaska requested review from webbnh and dbutenhof February 15, 2023 01:23

webbnh requested changes Feb 15, 2023

View reviewed changes

dbutenhof previously approved these changes Feb 15, 2023

View reviewed changes

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

dbutenhof reviewed Feb 15, 2023

View reviewed changes

npalaska dismissed dbutenhof’s stale review via 0d3351a February 15, 2023 21:23

review feedback

9f70672

npalaska force-pushed the pbench_1070 branch from 0d3351a to 9f70672 Compare February 15, 2023 21:38

dbutenhof approved these changes Feb 15, 2023

View reviewed changes

lib/pbench/server/auth/__init__.py Show resolved Hide resolved

lib/pbench/test/functional/server/test_connect.py Show resolved Hide resolved

webbnh previously approved these changes Feb 15, 2023

View reviewed changes

lib/pbench/client/oidc_admin.py Outdated Show resolved Hide resolved

lib/pbench/server/auth/__init__.py Outdated Show resolved Hide resolved

lib/pbench/server/auth/__init__.py Show resolved Hide resolved

lib/pbench/test/functional/server/test_connect.py Show resolved Hide resolved

dbutenhof requested changes Feb 16, 2023

View reviewed changes

lib/pbench/server/auth/__init__.py Show resolved Hide resolved

remove the internal test_user functional tests

b725ee7

npalaska dismissed webbnh’s stale review via b725ee7 February 16, 2023 15:09

npalaska requested review from dbutenhof and webbnh February 16, 2023 15:10

webbnh approved these changes Feb 16, 2023

View reviewed changes

lib/pbench/server/auth/__init__.py Show resolved Hide resolved

dbutenhof approved these changes Feb 16, 2023

View reviewed changes

lib/pbench/server/auth/__init__.py Show resolved Hide resolved

npalaska merged commit ffc8c5d into distributed-system-analysis:openid-connect Feb 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OIDC user to the functional test #3235

Add OIDC user to the functional test #3235

npalaska commented Feb 7, 2023

npalaska commented Feb 8, 2023

portante Feb 8, 2023

webbnh Feb 9, 2023

portante left a comment

portante Feb 8, 2023

npalaska Feb 8, 2023

npalaska Feb 13, 2023

npalaska Feb 20, 2023

portante Feb 20, 2023

npalaska Feb 21, 2023

dbutenhof left a comment

webbnh left a comment

webbnh Feb 9, 2023

npalaska commented Feb 9, 2023

portante commented Feb 9, 2023

npalaska commented Feb 9, 2023

dbutenhof commented Feb 9, 2023

portante left a comment

npalaska commented Feb 10, 2023

webbnh left a comment

webbnh left a comment

webbnh left a comment

		from requests.structures import CaseInsensitiveDict


		class OIDCAdmin:

Add OIDC user to the functional test #3235

Add OIDC user to the functional test #3235

Conversation

npalaska commented Feb 7, 2023

npalaska commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

portante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npalaska commented Feb 9, 2023

portante commented Feb 9, 2023

npalaska commented Feb 9, 2023

dbutenhof commented Feb 9, 2023

portante left a comment

Choose a reason for hiding this comment

npalaska commented Feb 10, 2023

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