diff --git a/dashboard/src/actions/overviewActions.js b/dashboard/src/actions/overviewActions.js index e91bf89335..f3f8051541 100644 --- a/dashboard/src/actions/overviewActions.js +++ b/dashboard/src/actions/overviewActions.js @@ -49,9 +49,7 @@ export const getDatasets = () => async (dispatch, getState) => { clearCachedSession(dispatch); } else { const msg = error.response?.data?.message; - dispatch( - showToast(DANGER, msg ? msg : `Error response: ERROR_MSG`) - ); + dispatch(showToast(DANGER, msg ? msg : `Error response: ERROR_MSG`)); dispatch({ type: TYPES.NETWORK_ERROR }); } } diff --git a/dashboard/src/utils/helper.js b/dashboard/src/utils/helper.js index 7d72ef8baf..ec0af30161 100644 --- a/dashboard/src/utils/helper.js +++ b/dashboard/src/utils/helper.js @@ -4,3 +4,19 @@ export const uid = () => { return head + tail; }; + +/** + * Expand a templated API URI like a Python `.format` + * + * @param {Object} endpoints - endpoint object from server + * @param {string} name - name of the API to expand + * @param {Object} args - value for each templated parameter + * @return {string} - formatted URI + */ +export const expandUriTemplate = (endpoints, name, args) => { + let uri = endpoints.uri[name].template; + for (const [key, value] of Object.entries(args)) { + uri = uri.replace(`{${key}}`, value); + } + return uri; +}; diff --git a/lib/pbench/cli/server/user_management.py b/lib/pbench/cli/server/user_management.py index 22df4b499c..804137d2eb 100644 --- a/lib/pbench/cli/server/user_management.py +++ b/lib/pbench/cli/server/user_management.py @@ -32,12 +32,7 @@ def user_list(context: object) -> None: users = User.query_all() for user in users: - click.echo( - USER_LIST_ROW_FORMAT.format( - user.username, - user.id, - ) - ) + click.echo(USER_LIST_ROW_FORMAT.format(user.username, user.id)) rv = 0 except Exception as exc: diff --git a/lib/pbench/client/__init__.py b/lib/pbench/client/__init__.py index 2911a80250..fa97d1558d 100644 --- a/lib/pbench/client/__init__.py +++ b/lib/pbench/client/__init__.py @@ -321,7 +321,6 @@ def connect(self, headers: Optional[dict[str, str]] = None) -> None: self.endpoints = response.json() assert self.endpoints - # Create an OIDCAdmin object and confirm the connection was successful self.oidc_admin = OIDCAdmin(server_url=self.endpoints["openid"]["server"]) def login(self, user: str, password: str): @@ -439,16 +438,6 @@ def get_list(self, **kwargs) -> Iterator[Dataset]: break json = self.get(uri=next_url).json() - def get_user(self, username: str, add_auth_header: bool = True) -> JSONOBJECT: - """ """ - if add_auth_header: - return self.get( - api=API.USER, uri_params={"target_username": username} - ).json() - response = self.session.get(self._uri(API.USER, {"target_username": username})) - response.raise_for_status() - return response.json() - def get_metadata(self, dataset_id: str, metadata: list[str]) -> JSONOBJECT: """Return requested metadata for a specified dataset. diff --git a/lib/pbench/server/api/resources/__init__.py b/lib/pbench/server/api/resources/__init__.py index ac267d2979..4032b192f3 100644 --- a/lib/pbench/server/api/resources/__init__.py +++ b/lib/pbench/server/api/resources/__init__.py @@ -1327,6 +1327,14 @@ def _check_authorization(self, mode: ApiAuthorization): role = mode.role authorized_user: User = Auth.token_auth.current_user() + username = "none" + if user_id: + user = User.query(id=user_id) + if user: + username = user.username + else: + current_app.logger.error("User ID {} not found", user_id) + # The ADMIN authorization doesn't involve a target resource owner or # access, so take care of that first as a special case. If there is # an authenticated user, and that user holds ADMIN access rights, the @@ -1348,9 +1356,10 @@ def _check_authorization(self, mode: ApiAuthorization): access = mode.access current_app.logger.debug( - "Authorizing {} access for {} to user (user id: {}) with access {} using {}", + "Authorizing {} access for {} to user {} ({}) with access {} using {}", role, authorized_user, + username, user_id, mode.access, mode.type, @@ -1379,12 +1388,12 @@ def _check_authorization(self, mode: ApiAuthorization): # An unauthenticated user is never allowed to access private # data nor to perform an potential mutation of data: REJECT current_app.logger.warning( - "Attempt to {} user {} data without login", role, user_id + "Attempt to {} user {} data without login", role, username ) raise UnauthorizedAccess( authorized_user, role, - user_id, + username, access, HTTPStatus.UNAUTHORIZED, ) @@ -1397,7 +1406,7 @@ def _check_authorization(self, mode: ApiAuthorization): role, ) raise UnauthorizedAccess( - authorized_user, role, user_id, access, HTTPStatus.FORBIDDEN + authorized_user, role, username, access, HTTPStatus.FORBIDDEN ) elif ( user_id @@ -1411,10 +1420,10 @@ def _check_authorization(self, mode: ApiAuthorization): "Unauthorized attempt by {} to {} user {} data", authorized_user, role, - user_id, + username, ) raise UnauthorizedAccess( - authorized_user, role, user_id, access, HTTPStatus.FORBIDDEN + authorized_user, role, username, access, HTTPStatus.FORBIDDEN ) else: # We have determined that there is an authenticated user with diff --git a/lib/pbench/server/api/resources/datasets_list.py b/lib/pbench/server/api/resources/datasets_list.py index 2d1aefdd41..375ffe5857 100644 --- a/lib/pbench/server/api/resources/datasets_list.py +++ b/lib/pbench/server/api/resources/datasets_list.py @@ -458,7 +458,7 @@ def _get( if key == Metadata.USER: if not auth_id: continue - terms.append(table.user_id == auth_id) + terms.append(table.user_ref == auth_id) query = query.outerjoin(table, and_(*terms)) if "start" in json and "end" in json: diff --git a/lib/pbench/server/api/resources/datasets_metadata.py b/lib/pbench/server/api/resources/datasets_metadata.py index 385e7e4061..fcce2c418e 100644 --- a/lib/pbench/server/api/resources/datasets_metadata.py +++ b/lib/pbench/server/api/resources/datasets_metadata.py @@ -23,7 +23,6 @@ Metadata, MetadataBadValue, MetadataError, - User, ) diff --git a/lib/pbench/server/api/resources/endpoint_configure.py b/lib/pbench/server/api/resources/endpoint_configure.py index 355c5a25d6..21e673f5b3 100644 --- a/lib/pbench/server/api/resources/endpoint_configure.py +++ b/lib/pbench/server/api/resources/endpoint_configure.py @@ -178,9 +178,9 @@ def get(self): } try: - client = self.server_config.get("openid-connect", "client") - realm = self.server_config.get("openid-connect", "realm") - server = self.server_config.get("openid-connect", "server_url") + client = self.server_config.get("openid", "client") + realm = self.server_config.get("openid", "realm") + server = self.server_config.get("openid", "server_url") except (NoOptionError, NoSectionError): pass else: diff --git a/lib/pbench/server/api/resources/users_api.py b/lib/pbench/server/api/resources/users_api.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/lib/pbench/server/auth/__init__.py b/lib/pbench/server/auth/__init__.py index 7e2856d5e6..cc769403eb 100644 --- a/lib/pbench/server/auth/__init__.py +++ b/lib/pbench/server/auth/__init__.py @@ -262,9 +262,9 @@ def construct_oidc_client(cls, server_config: PbenchServerConfig) -> "OpenIDClie missing, or any of the required options are missing. """ try: - server_url = server_config.get("openid-connect", "server_url") - client = server_config.get("openid-connect", "client") - realm = server_config.get("openid-connect", "realm") + server_url = server_config.get("openid", "server_url") + client = server_config.get("openid", "client") + realm = server_config.get("openid", "realm") except (NoOptionError, NoSectionError) as exc: raise OpenIDClient.NotConfigured() from exc diff --git a/lib/pbench/server/database/alembic/env.py b/lib/pbench/server/database/alembic/env.py index 771ddf26d4..3664074207 100644 --- a/lib/pbench/server/database/alembic/env.py +++ b/lib/pbench/server/database/alembic/env.py @@ -60,7 +60,9 @@ def run_migrations_online() -> None: with connectable.connect() as connection: context.configure( - connection=connection, target_metadata=target_metadata, compare_type=True + connection=connection, + target_metadata=target_metadata, + compare_type=True, ) with context.begin_transaction(): diff --git a/lib/pbench/server/database/alembic/versions/f628657bed56_user_table_update_oidc.py b/lib/pbench/server/database/alembic/versions/f628657bed56_user_table_update_oidc.py index 99fcbcfc66..d169836bc2 100644 --- a/lib/pbench/server/database/alembic/versions/f628657bed56_user_table_update_oidc.py +++ b/lib/pbench/server/database/alembic/versions/f628657bed56_user_table_update_oidc.py @@ -11,7 +11,7 @@ # revision identifiers, used by Alembic. revision = "f628657bed56" -down_revision = "fa12f45a2a5a" +down_revision = "9cc638cb865c" branch_labels = None depends_on = None @@ -26,9 +26,6 @@ def upgrade() -> None: type_=sa.String(length=255), existing_nullable=False, ) - op.alter_column( - "users", "username", existing_type=sa.VARCHAR(length=255), nullable=False - ) op.create_primary_key("user_primary", "users", ["id"]) op.drop_constraint("users_email_key", "users", type_="unique") op.drop_column("users", "role") @@ -37,7 +34,7 @@ def upgrade() -> None: op.drop_column("users", "last_name") op.drop_column("users", "email") op.drop_column("users", "registered_on") - op.drop_column("active_tokens", "user_id") + op.drop_column("auth_tokens", "user_id") op.add_column("dataset_metadata", sa.Column("user_ref", sa.String(), nullable=True)) op.create_foreign_key(None, "dataset_metadata", "users", ["user_ref"], ["id"]) op.drop_column("dataset_metadata", "user_id") @@ -83,9 +80,6 @@ def downgrade() -> None: ), ) op.create_unique_constraint("users_email_key", "users", ["email"]) - op.alter_column( - "users", "username", existing_type=sa.VARCHAR(length=255), nullable=False - ) op.alter_column( "users", "id", @@ -103,12 +97,12 @@ def downgrade() -> None: ) op.drop_column("dataset_metadata", "user_ref") op.add_column( - "active_tokens", + "auth_tokens", sa.Column("user_id", sa.INTEGER(), autoincrement=False, nullable=False), ) op.create_foreign_key( - "active_tokens_user_id_fkey", - "active_tokens", + "auth_tokens_user_id_fkey", + "auth_tokens", "users", ["user_id"], ["id"], diff --git a/lib/pbench/server/database/models/active_tokens.py b/lib/pbench/server/database/models/active_tokens.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/lib/pbench/server/database/models/auth_tokens.py b/lib/pbench/server/database/models/auth_tokens.py index c282e68af1..cc5c6e9aee 100644 --- a/lib/pbench/server/database/models/auth_tokens.py +++ b/lib/pbench/server/database/models/auth_tokens.py @@ -1,7 +1,6 @@ from typing import Optional -from sqlalchemy import Column, DateTime, ForeignKey, Integer, String -from sqlalchemy.orm import relationship +from sqlalchemy import Column, DateTime, Integer, String from pbench.server.database.database import Database @@ -17,13 +16,6 @@ class AuthToken(Database.Base): id = Column(Integer, primary_key=True, autoincrement=True) token = Column(String(500), unique=True, nullable=False, index=True) expiration = Column(DateTime, nullable=False, index=True) - user_id = Column( - Integer, - ForeignKey("users.id", ondelete="CASCADE"), - nullable=False, - # no need to add index=True, all FKs have indexes - ) - user = relationship("User", back_populates="auth_tokens") @staticmethod def query(auth_token: str) -> Optional["AuthToken"]: diff --git a/lib/pbench/server/database/models/users.py b/lib/pbench/server/database/models/users.py index 2008326f02..93971735e4 100644 --- a/lib/pbench/server/database/models/users.py +++ b/lib/pbench/server/database/models/users.py @@ -44,7 +44,7 @@ def __init__(self, user: "User", cause: str): self.cause = cause def __str__(self) -> str: - return f"Duplicate user setting in {self.user.get_json()}: {self.cause}" + return f"Duplicate user entry in {self.user.get_json()}: {self.cause}" class UserNullKey(UserError): @@ -95,7 +95,9 @@ def get_json(self) -> JSONOBJECT: """ return {"username": self.username, "id": self.id, "roles": self.roles} - def _decode(self, exception: IntegrityError) -> Exception: + def _decode( + self, exception: IntegrityError, operation: Optional[str] = "" + ) -> Exception: """Decode a SQLAlchemy IntegrityError to look for a recognizable UNIQUE or NOT NULL constraint violation. @@ -114,7 +116,8 @@ def _decode(self, exception: IntegrityError) -> Exception: return UserDuplicate(self, cause) elif "NOT NULL constraint" in cause: return UserNullKey(self, cause) - return exception + else: + return UserSqlError(operation, self.get_json(), str(exception)) @staticmethod def query(id: str = None, username: str = None) -> Optional["User"]: @@ -146,9 +149,7 @@ def add(self): Database.db_session.commit() except Exception as e: Database.db_session.rollback() - if isinstance(e, IntegrityError): - raise self._decode(e) from e - raise UserSqlError("adding", self.get_json(), str(e)) from e + raise self._decode(e, "adding") from e def update(self, **kwargs): """Update the current user object with given keyword arguments.""" @@ -158,18 +159,7 @@ def update(self, **kwargs): Database.db_session.commit() except Exception as e: Database.db_session.rollback() - if isinstance(e, IntegrityError): - raise self._decode(e) from e - raise UserSqlError("Updating", self.get_json(), str(e)) from e - - def delete(self): - """Delete the user with a given username, except admin.""" - try: - Database.db_session.delete(self) - Database.db_session.commit() - except Exception as e: - Database.db_session.rollback() - raise UserSqlError("deleting", self.get_json(), str(e)) from e + raise self._decode(e, "updating") from e def is_admin(self) -> bool: """This method checks whether the given user has an admin role. diff --git a/lib/pbench/test/functional/server/conftest.py b/lib/pbench/test/functional/server/conftest.py index 618bd9bd49..cc2181f61e 100644 --- a/lib/pbench/test/functional/server/conftest.py +++ b/lib/pbench/test/functional/server/conftest.py @@ -5,6 +5,7 @@ from pbench.client import PbenchServerClient from pbench.client.oidc_admin import OIDCAdmin +from pbench.server.auth import OpenIDClientError USERNAME: str = "tester" EMAIL: str = "tester@gmail.com" @@ -35,7 +36,7 @@ def server_client(): def oidc_admin(server_client: PbenchServerClient): """ Used by Pbench Server functional tests to get admin access - on OIDC server. + on the test OIDC server. """ return OIDCAdmin(server_url=server_client.endpoints["openid"]["server"]) @@ -43,19 +44,22 @@ def oidc_admin(server_client: PbenchServerClient): @pytest.fixture(scope="session") def register_test_user(oidc_admin: OIDCAdmin): """Create a test user for functional tests.""" - response = oidc_admin.create_new_user( - username=USERNAME, - email=EMAIL, - password=PASSWORD, - first_name=FIRST_NAME, - last_name=LAST_NAME, - ) - - # To allow testing outside our transient CI containers, allow the tester - # user to already exist. - assert ( - response.ok or response.status_code == HTTPStatus.CONFLICT - ), f"Register failed with {response.json()}" + try: + response = oidc_admin.create_new_user( + username=USERNAME, + email=EMAIL, + password=PASSWORD, + first_name=FIRST_NAME, + last_name=LAST_NAME, + ) + except OpenIDClientError as e: + # To allow testing outside our transient CI containers, allow the tester + # user to already exist. + if e.http_status != HTTPStatus.CONFLICT: + raise e + pass + else: + assert response.ok, f"Register failed with {response.json()}" @pytest.fixture @@ -63,3 +67,5 @@ def login_user(server_client: PbenchServerClient, register_test_user): """Log in the test user and return the authentication token""" server_client.login(USERNAME, PASSWORD) assert server_client.auth_token + yield + server_client.auth_token = None diff --git a/lib/pbench/test/functional/server/test_connect.py b/lib/pbench/test/functional/server/test_connect.py index b0f0664121..01563329a3 100644 --- a/lib/pbench/test/functional/server/test_connect.py +++ b/lib/pbench/test/functional/server/test_connect.py @@ -28,6 +28,5 @@ def test_connect(self, server_client: PbenchServerClient): assert e in endpoints["uri"].keys() # verify all the required openid-connect fields are present - if "openid" in endpoints: - expected = {"client", "realm", "server"} - assert set(endpoints["openid"]) >= expected + oid_ep = set(endpoints.get("openid", {})) + assert oid_ep >= {"client", "realm", "server"} or not oid_ep diff --git a/lib/pbench/test/unit/server/auth/test_auth.py b/lib/pbench/test/unit/server/auth/test_auth.py index 17f042e7c6..36063a360f 100644 --- a/lib/pbench/test/unit/server/auth/test_auth.py +++ b/lib/pbench/test/unit/server/auth/test_auth.py @@ -240,7 +240,7 @@ def mock_connection( realm_name = "camelot" secret = "shhh" config = configparser.ConfigParser() - config["openid-connect"] = { + config["openid"] = { "server_url": server_url, "client": client_id, "realm": realm_name, @@ -293,14 +293,14 @@ class TestOpenIDClient: def test_construct_oidc_client_fail(self): """Verfiy .construct_oidc_client() failure mode""" - # Start with an empty configuration, no openid-connect section + # Start with an empty configuration, no openid section config = configparser.ConfigParser() with pytest.raises(OpenIDClient.NotConfigured): OpenIDClient.construct_oidc_client(config) # Missing "server_url" section = {} - config["openid-connect"] = section + config["openid"] = section with pytest.raises(OpenIDClient.NotConfigured): OpenIDClient.construct_oidc_client(config) @@ -322,8 +322,8 @@ def test_construct_oidc_client_succ(self, monkeypatch): client_id = "us" public_key = "hijklmn" config = mock_connection(monkeypatch, client_id, public_key) - server_url = config["openid-connect"]["server_url"] - realm_name = config["openid-connect"]["realm"] + server_url = config["openid"]["server_url"] + realm_name = config["openid"]["realm"] oidc_client = OpenIDClient.construct_oidc_client(config) @@ -569,19 +569,17 @@ def test_verify_auth_oidc(self, monkeypatch, rsa_keys, make_logger, roles): oidc_client = OpenIDClient.construct_oidc_client(config) monkeypatch.setattr(Auth, "oidc_client", oidc_client) - app = Flask("test-verify-auth-oidc-offline") + app = Flask("test-verify-auth-oidc") app.logger = make_logger with app.app_context(): user = Auth.verify_auth(token) assert user.id == "12345" - if roles is not None: - assert user.roles == roles - else: - assert user.roles == [] + assert user.roles == (roles if roles else []) def test_verify_auth_oidc_user_update(self, monkeypatch, rsa_keys, make_logger): - """Verify OIDC token offline verification success path""" + """Verify we update our internal user database when we get updated user + payload from the OIDC token for an existing user.""" client_id = "us" token, expected_payload = gen_rsa_token(client_id, rsa_keys["private_key"]) @@ -593,13 +591,14 @@ def test_verify_auth_oidc_user_update(self, monkeypatch, rsa_keys, make_logger): oidc_client = OpenIDClient.construct_oidc_client(config) monkeypatch.setattr(Auth, "oidc_client", oidc_client) - app = Flask("test-verify-auth-oidc-offline") + app = Flask("test-verify-auth-oidc-user-update") app.logger = make_logger with app.app_context(): user = Auth.verify_auth(token) assert user.id == "12345" assert user.roles == [] + assert user.username == "dummy" # Generate a new token with a role for the same user token, expected_payload = gen_rsa_token( @@ -632,7 +631,7 @@ def test_verify_auth_oidc_invalid(self, monkeypatch, rsa_keys, make_logger): def tio_exc(token: str) -> JSON: raise OpenIDTokenInvalid() - app = Flask("test-verify-auth-oidc-offline-invalid") + app = Flask("test-verify-auth-oidc-invalid") app.logger = make_logger with app.app_context(): monkeypatch.setattr(oidc_client, "token_introspect", tio_exc) diff --git a/lib/pbench/test/unit/server/conftest.py b/lib/pbench/test/unit/server/conftest.py index 0ead6d8d8e..d12217f615 100644 --- a/lib/pbench/test/unit/server/conftest.py +++ b/lib/pbench/test/unit/server/conftest.py @@ -28,7 +28,6 @@ import pbench.server.auth.auth as Auth from pbench.server.database import init_db from pbench.server.database.database import Database -from pbench.server.database.models.auth_tokens import AuthToken from pbench.server.database.models.datasets import Dataset, Metadata from pbench.server.database.models.templates import Template from pbench.server.database.models.users import User @@ -48,7 +47,7 @@ [flask-app] secret-key = my_precious -[openid-connect] +[openid] server_url = http://openid.example.com [logging] @@ -163,8 +162,8 @@ def add_auth_connection_mock(server_config, rsa_keys): rsa_keys: rsa_keys fixture to get te public key """ with responses.RequestsMock() as mock: - oidc_server = server_config.get("openid-connect", "server_url") - oidc_realm = server_config.get("openid-connect", "realm") + oidc_server = server_config.get("openid", "server_url") + oidc_realm = server_config.get("openid", "realm") url = urljoin(oidc_server, f"realms/{oidc_realm}") mock.add( @@ -775,7 +774,7 @@ def pbench_admin_token(client, server_config, create_admin_user, rsa_keys): return generate_token( user=create_admin_user, private_key=rsa_keys["private_key"], - client_id=server_config.get("openid-connect", "client"), + client_id=server_config.get("openid", "client"), username=admin_username, pbench_client_roles=["ADMIN"], ) @@ -786,7 +785,7 @@ def pbench_drb_token(client, server_config, create_drb_user, rsa_keys): """OIDC valid token for the 'drb' user""" return generate_token( username="drb", - client_id=server_config.get("openid-connect", "client"), + client_id=server_config.get("openid", "client"), private_key=rsa_keys["private_key"], user=create_drb_user, ) @@ -798,7 +797,7 @@ def pbench_drb_token_invalid(client, server_config, create_drb_user, rsa_keys): return generate_token( username="drb", private_key=rsa_keys["private_key"], - client_id=server_config.get("openid-connect", "client"), + client_id=server_config.get("openid", "client"), user=create_drb_user, valid=False, ) @@ -821,7 +820,7 @@ def get_token_func(pbench_admin_token, server_config, rsa_keys): else generate_token( username=user, private_key=rsa_keys["private_key"], - client_id=server_config.get("openid-connect", "client"), + client_id=server_config.get("openid", "client"), ) ) @@ -869,27 +868,10 @@ def generate_token( "sub": user.id, "aud": client_id, "azp": client_id, - "realm_access": { - "roles": [ - "default-roles-pbench-server", - "offline_access", - "uma_authorization", - ] - }, - "resource_access": { - "broker": {"roles": ["read-token"]}, - "account": { - "roles": ["manage-account", "manage-account-links", "view-profile"] - }, - }, + "resource_access": {}, "scope": "openid profile email", "sid": "1988612e-774d-43b8-8d4a-bbc05ee55edb", - "email_verified": True, - "name": "first_name last_name", "preferred_username": username, - "given_name": "first_name", - "family_name": "last_name", - "email": "dummy@example.com", } if pbench_client_roles: payload["resource_access"].update({client_id: {"roles": pbench_client_roles}}) diff --git a/lib/pbench/test/unit/server/database/__init__.py b/lib/pbench/test/unit/server/database/__init__.py index 6afb0ffc74..16ec2666ae 100644 --- a/lib/pbench/test/unit/server/database/__init__.py +++ b/lib/pbench/test/unit/server/database/__init__.py @@ -222,7 +222,7 @@ def filter(self, *criteria) -> "FakeQuery": def count(self) -> int: """Returns the number of results filter query returned""" - return int(len(self.session.filters)) + return len(self.session.filters) def order_by(self, column: Column) -> "FakeQuery": """Sort the currently selected records by a specified column""" @@ -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 + are performed after updates and adds, therefore delete-then-add won't + work as expected. """ if self.raise_on_commit: exc = self.raise_on_commit @@ -320,8 +324,9 @@ def commit(self): for k, object in self.known.items(): self.committed[k] = FakeRow.clone(object) for a in self.added: - a.id = self.id if not a.id else a.id - self.id += 1 + if not a.id: + a.id = self.id + self.id += 1 for c in a.__table__._columns: if c.default: default = c.default @@ -398,7 +403,7 @@ def check_session( # Check that the 'committed' list (which stands in for the actual DB # table) contains the expected rows. - assert committed is None or all(i in self.committed.values() for i in committed) + assert committed is None or sorted(self.committed.values()) == sorted(committed) # Check whether we've rolled back transaction(s). assert self.rolledback == rolledback diff --git a/lib/pbench/test/unit/server/database/test_datasets_db.py b/lib/pbench/test/unit/server/database/test_datasets_db.py index fe8445622b..6d4fd4fe4f 100644 --- a/lib/pbench/test/unit/server/database/test_datasets_db.py +++ b/lib/pbench/test/unit/server/database/test_datasets_db.py @@ -25,17 +25,6 @@ def test_construct(self, db_session, create_user): "operations": {}, } - def test_dataset_survives_user(self, db_session, create_user): - """The Dataset isn't automatically removed when the referenced - user is removed. - """ - user = create_user - ds = Dataset(owner=user, name="fio", resource_id="deadbeef") - ds.add() - user.delete() - ds1 = Dataset.query(resource_id="deadbeef") - assert ds1 == ds - def test_dataset_metadata_log(self, db_session, create_user, provide_metadata): """ Test that `as_dict` provides the mocked metadata.log contents along diff --git a/lib/pbench/test/unit/server/database/test_datasets_metadata_db.py b/lib/pbench/test/unit/server/database/test_datasets_metadata_db.py index 01f9a22821..0eaeac2097 100644 --- a/lib/pbench/test/unit/server/database/test_datasets_metadata_db.py +++ b/lib/pbench/test/unit/server/database/test_datasets_metadata_db.py @@ -446,15 +446,15 @@ def test_mutable_origin(self, server_config, attach_dataset): [ ( True, - "Metadata key 'server.origin' value True for dataset (3)|drb must be a string", + "Metadata key 'server.origin' value True for dataset (drb)|drb must be a string", ), ( [], - "Metadata key 'server.origin' value [] for dataset (3)|drb must be a string", + "Metadata key 'server.origin' value [] for dataset (drb)|drb must be a string", ), ( 1, - "Metadata key 'server.origin' value 1 for dataset (3)|drb must be a string", + "Metadata key 'server.origin' value 1 for dataset (drb)|drb must be a string", ), ], ) @@ -494,19 +494,19 @@ def test_mutable_archiveonly(self, server_config, attach_dataset, value, result) [ ( "ABC", - "Metadata key 'server.archiveonly' value 'ABC' for dataset (3)|drb must be a boolean", + "Metadata key 'server.archiveonly' value 'ABC' for dataset (drb)|drb must be a boolean", ), ( "Truth", - "Metadata key 'server.archiveonly' value 'Truth' for dataset (3)|drb must be a boolean", + "Metadata key 'server.archiveonly' value 'Truth' for dataset (drb)|drb must be a boolean", ), ( [], - "Metadata key 'server.archiveonly' value [] for dataset (3)|drb must be a boolean", + "Metadata key 'server.archiveonly' value [] for dataset (drb)|drb must be a boolean", ), ( 1.00, - "Metadata key 'server.archiveonly' value 1.0 for dataset (3)|drb must be a boolean", + "Metadata key 'server.archiveonly' value 1.0 for dataset (drb)|drb must be a boolean", ), ], ) diff --git a/lib/pbench/test/unit/server/database/test_users_db.py b/lib/pbench/test/unit/server/database/test_users_db.py index 29a06d1697..b24751e04c 100644 --- a/lib/pbench/test/unit/server/database/test_users_db.py +++ b/lib/pbench/test/unit/server/database/test_users_db.py @@ -5,7 +5,7 @@ from pbench.server.database.database import Database from pbench.server.database.models.datasets import Dataset, DatasetNotFound -from pbench.server.database.models.users import User, UserDuplicate, UserSqlError +from pbench.server.database.models.users import User, UserDuplicate from pbench.test.unit.server.database import FakeDBOrig, FakeRow, FakeSession @@ -68,7 +68,7 @@ def test_user_survives_dataset_real_session(self, db_session, create_user): ds.delete() with pytest.raises(DatasetNotFound): Dataset.query(resource_id=ds.resource_id) - assert User.query(username=user.username) + assert user == User.query(username=user.username) def test_construct_duplicate(self, fake_db): """Test handling of User record duplicate value error""" @@ -77,7 +77,7 @@ def test_construct_duplicate(self, fake_db): ) with pytest.raises( UserDuplicate, - match="Duplicate user setting in {'username': 'dummy', 'id': .*?, 'roles': \\[]}: UNIQUE constraint", + match="Duplicate user entry in {'username': 'dummy', 'id': [^,]+, 'roles': \\[]}: UNIQUE constraint", ): self.add_dummy_user(fake_db) self.session.check_session(rolledback=1) @@ -86,11 +86,10 @@ def test_construct_duplicate(self, fake_db): def test_user_update(self, fake_db): """Test updating user roles""" - data = {"roles": ["NEW_ROLE"]} TestUsers.add_dummy_user(fake_db) user = User.query(username="dummy") - user.update(**data) + user.update(roles=["NEW_ROLE"]) assert user.roles == ["NEW_ROLE"] assert user._roles == "NEW_ROLE" @@ -99,28 +98,3 @@ def test_user_update(self, fake_db): queries=1, committed=expected_commits, filters=["username=dummy"] ) self.session.reset_context() - - def test_user_delete(self, fake_db): - """Test deleting the user from the User table""" - self.add_dummy_user(fake_db) - user = User.query(username="dummy") - expected_commits = [FakeRow.clone(user)] - self.session.check_session( - queries=1, filters=["username=dummy"], committed=expected_commits - ) - assert user.username == "dummy" - user.delete() - self.session.check_session(queries=0, filters=[], committed=[]) - - def test_delete_exception(self, fake_db): - """Test exception raised during the delete operation""" - user = User( - id="1", - username="dummy", - ) - with pytest.raises( - UserSqlError, - match=r"deleting", - ): - user.delete() - self.session.reset_context() diff --git a/lib/pbench/test/unit/server/query_apis/test_datasets_detail.py b/lib/pbench/test/unit/server/query_apis/test_datasets_detail.py index 28e5764d37..9f9830a467 100644 --- a/lib/pbench/test/unit/server/query_apis/test_datasets_detail.py +++ b/lib/pbench/test/unit/server/query_apis/test_datasets_detail.py @@ -7,7 +7,6 @@ DatasetsDetail, ) from pbench.server.database.models.datasets import Dataset, Metadata -from pbench.test.unit.server.conftest import generate_token from pbench.test.unit.server.query_apis.commons import Commons @@ -47,10 +46,9 @@ def test_query( server_config, query_api, find_template, - pbench_admin_token, - rsa_keys, user, expected_status, + get_token_func, ): """ Check the construction of Elasticsearch query URI and filtering of the response body. @@ -60,14 +58,7 @@ def test_query( headers = None if user: - if user == "test_admin": - token = pbench_admin_token - else: - token = generate_token( - username=user, - private_key=rsa_keys["private_key"], - client_id=server_config.get("openid-connect", "client"), - ) + token = get_token_func(user) assert token headers = {"authorization": f"bearer {token}"} diff --git a/lib/pbench/test/unit/server/test_authorization.py b/lib/pbench/test/unit/server/test_authorization.py index 8639fa5ec3..d9a6ae3122 100644 --- a/lib/pbench/test/unit/server/test_authorization.py +++ b/lib/pbench/test/unit/server/test_authorization.py @@ -70,7 +70,7 @@ def test_disallowed_admin(self, apibase, server_config, current_user_admin, ask) ApiAuthorizationType.USER_ACCESS, ask["role"], user, access ) ) - assert exc.value.owner == (user if ask["user"] else None) + assert exc.value.owner == (ask["user"] if ask["user"] else "none") assert exc.value.user == current_user_admin @pytest.mark.parametrize( @@ -128,7 +128,7 @@ def test_disallowed_auth( ApiAuthorizationType.USER_ACCESS, ask["role"], user, access ) ) - assert exc.value.owner == (user if ask["user"] else None) + assert exc.value.owner == (ask["user"] if ask["user"] else "none") assert exc.value.user == current_user_drb @pytest.mark.parametrize( @@ -176,7 +176,7 @@ def test_disallowed_noauth( ApiAuthorizationType.USER_ACCESS, ask["role"], user, access ) ) - assert exc.value.owner == (user if ask["user"] else None) + assert exc.value.owner == (ask["user"] if ask["user"] else "none") assert exc.value.user is None def test_admin_unauth(self, apibase, server_config, current_user_none): diff --git a/lib/pbench/test/unit/server/test_datasets_inventory.py b/lib/pbench/test/unit/server/test_datasets_inventory.py index 1dc028f823..5248b9043a 100644 --- a/lib/pbench/test/unit/server/test_datasets_inventory.py +++ b/lib/pbench/test/unit/server/test_datasets_inventory.py @@ -7,7 +7,6 @@ from pbench.server.cache_manager import CacheManager from pbench.server.database.models.datasets import Dataset, DatasetNotFound -from pbench.test.unit.server import TEST_USER_ID class TestDatasetsAccess: @@ -66,7 +65,7 @@ def test_dataset_not_present(self, query_get_as): def test_unauthorized_access(self, query_get_as): response = query_get_as("test", "metadata.log", HTTPStatus.FORBIDDEN) assert response.json == { - "message": f"User drb is not authorized to READ a resource owned by {TEST_USER_ID} with private access" + "message": "User drb is not authorized to READ a resource owned by test with private access" } def test_dataset_is_not_unpacked(self, query_get_as, monkeypatch): diff --git a/lib/pbench/test/unit/server/test_datasets_list.py b/lib/pbench/test/unit/server/test_datasets_list.py index 7e08e64bd2..e2aa70e45b 100644 --- a/lib/pbench/test/unit/server/test_datasets_list.py +++ b/lib/pbench/test/unit/server/test_datasets_list.py @@ -67,7 +67,7 @@ def filter_setup(self, auth_id: Optional[str]): if key == Metadata.USER: if not auth_id: continue - terms.append(table.user_id == auth_id) + terms.append(table.user_ref == auth_id) query = query.outerjoin(table, and_(*terms)) return aliases, query @@ -525,7 +525,7 @@ def test_filter_query(self, monkeypatch, client, filters, expected): "FROM datasets LEFT OUTER JOIN dataset_metadata AS dataset_metadata_1 ON dataset_metadata_1.dataset_ref = datasets.id AND dataset_metadata_1.key = 'metalog' " "LEFT OUTER JOIN dataset_metadata AS dataset_metadata_2 ON dataset_metadata_2.dataset_ref = datasets.id AND dataset_metadata_2.key = 'server' " "LEFT OUTER JOIN dataset_metadata AS dataset_metadata_3 ON dataset_metadata_3.dataset_ref = datasets.id AND dataset_metadata_3.key = 'global' " - "LEFT OUTER JOIN dataset_metadata AS dataset_metadata_4 ON dataset_metadata_4.dataset_ref = datasets.id AND dataset_metadata_4.key = 'user' AND dataset_metadata_4.user_id = '3' WHERE " + "LEFT OUTER JOIN dataset_metadata AS dataset_metadata_4 ON dataset_metadata_4.dataset_ref = datasets.id AND dataset_metadata_4.key = 'user' AND dataset_metadata_4.user_ref = '3' WHERE " ) aliases, query = self.filter_setup(DRB_USER_ID) query = DatasetsList.filter_query(filters, aliases, query) diff --git a/lib/pbench/test/unit/server/test_endpoint_configure.py b/lib/pbench/test/unit/server/test_endpoint_configure.py index 20ed29242b..96a369faa9 100644 --- a/lib/pbench/test/unit/server/test_endpoint_configure.py +++ b/lib/pbench/test/unit/server/test_endpoint_configure.py @@ -123,9 +123,9 @@ def check_config(self, client, server_config, host, my_headers={}): } try: - oidc_client = server_config.get("openid-connect", "client") - oidc_realm = server_config.get("openid-connect", "realm") - oidc_server = server_config.get("openid-connect", "server_url") + oidc_client = server_config.get("openid", "client") + oidc_realm = server_config.get("openid", "realm") + oidc_server = server_config.get("openid", "server_url") except (NoOptionError, NoSectionError): pass else: diff --git a/lib/pbench/test/unit/server/test_schema.py b/lib/pbench/test/unit/server/test_schema.py index 20babe2d79..46646efa4f 100644 --- a/lib/pbench/test/unit/server/test_schema.py +++ b/lib/pbench/test/unit/server/test_schema.py @@ -171,8 +171,8 @@ def test_unauthenticated_username(self, monkeypatch, current_user_none): client is unauthenticated even when the username exists. """ user = User( - username="drb", id="1", + username="drb", ) def ok(username: str) -> User: diff --git a/lib/pbench/test/unit/server/test_shell_cli.py b/lib/pbench/test/unit/server/test_shell_cli.py index 9ac5d67f00..14a28405d9 100644 --- a/lib/pbench/test/unit/server/test_shell_cli.py +++ b/lib/pbench/test/unit/server/test_shell_cli.py @@ -268,16 +268,13 @@ def test_main_crontab_failed(monkeypatch, make_logger, mock_get_server_config): def immediate_success(*args, **kwargs): pass - def wait_for_oidc_server( - server_config: PbenchServerConfig, logger: logging.Logger - ) -> str: - return "https://oidc.example.com" - def generate_crontab_if_necessary(*args, **kwargs) -> int: return 43 monkeypatch.setattr( - shell.OpenIDClient, "wait_for_oidc_server", wait_for_oidc_server + shell.OpenIDClient, + "wait_for_oidc_server", + lambda config, logger: "https://oidc.example.com", ) monkeypatch.setattr(shell.site, "ENABLE_USER_SITE", False) monkeypatch.setattr(shell, "wait_for_uri", immediate_success) diff --git a/lib/pbench/test/unit/server/test_user_auth.py b/lib/pbench/test/unit/server/test_user_auth.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/server/lib/config/pbench-server-default.cfg b/server/lib/config/pbench-server-default.cfg index f64cf8a756..6e3b074df6 100644 --- a/server/lib/config/pbench-server-default.cfg +++ b/server/lib/config/pbench-server-default.cfg @@ -88,7 +88,7 @@ wait_timeout = 120 # related tasks. #secret-key = -[openid-connect] +[openid] # OpenID Connect (OIDC) section to use when OIDC is to be used as the user # identity provider for authentication and authorization purposes. The OIDC # server we'll expect to use is KeyCloak. @@ -101,7 +101,7 @@ wait_timeout = 120 realm = pbench-server # Client entity name requesting OIDC to authenticate a user. -client = pbench-dashboard +client = pbench-client [logging] logger_type = devlog diff --git a/server/pbenchinacan/etc/pbench-server/pbench-server.cfg b/server/pbenchinacan/etc/pbench-server/pbench-server.cfg index 896d6c7665..05468ce5e0 100644 --- a/server/pbenchinacan/etc/pbench-server/pbench-server.cfg +++ b/server/pbenchinacan/etc/pbench-server/pbench-server.cfg @@ -23,11 +23,8 @@ uri = postgresql://pbenchcontainer:pbench@localhost:5432/pbenchcontainer [flask-app] secret-key = "pbench-in-a-can secret shhh" -[openid-connect] +[openid] server_url = http://localhost:8090 -secret = keycloak_secret -realm = -client = ########################################################################### # The rest will come from the default config file. diff --git a/server/pbenchinacan/load_keycloak.sh b/server/pbenchinacan/load_keycloak.sh index 7c23c5a186..46f5f75a3d 100755 --- a/server/pbenchinacan/load_keycloak.sh +++ b/server/pbenchinacan/load_keycloak.sh @@ -1,10 +1,10 @@ #!/bin/bash -e # This script configures a running Keycloak server with following configuration -# 1. A realm with the name 'pbench' -# 2. A client 'pbench-server' under the realm pbench +# 1. A realm with the name 'pbench-server' +# 2. A client 'pbench-client' under the realm pbench-server # 3. A user 'admin' under the realm pbench -# 4. An 'ADMIN' role under the client pbench-server +# 4. An 'ADMIN' role under the client pbench-client # 5. ADMIN role assigned to the admin user. # This file uses a realm of 'pbench' and a client name of 'pbench-server' @@ -24,7 +24,7 @@ ADMIN_PASSWORD=${ADMIN_PASSWORD:-"admin"} # These values must match the options "realm" and "client in the # "openid-connect" section of the pbench server configuration file. REALM=${KEYCLOAK_REALM:-"pbench-server"} -CLIENT=${KEYCLOAK_CLIENT:-"pbench-dashboard"} +CLIENT=${KEYCLOAK_CLIENT:-"pbench-client"} end_in_epoch_secs=$(date --date "2 minutes" +%s) @@ -66,7 +66,7 @@ else fi # Create a client scope with custom mapper that will instruct Keycloak -# to include the (pbench-dashboard) when someone requests +# to include the (pbench-client) when someone requests # a token from Keycloak using a . # Having in the aud claim of the token is essential for the token # to be validated. @@ -102,7 +102,13 @@ curl -si -f -X POST "${KEYCLOAK_HOST_PORT}/admin/realms/${REALM}/client-scopes" CLIENT_CONF=$(curl -si -f -X POST "${KEYCLOAK_HOST_PORT}/admin/realms/${REALM}/clients" \ -H "Authorization: Bearer ${ADMIN_TOKEN}" \ -H "Content-Type: application/json" \ - -d '{"clientId": "'${CLIENT}'", "publicClient": true, "defaultClientScopes": ["pbench", "openid", "profile", "email"], "directAccessGrantsEnabled": true, "serviceAccountsEnabled": true, "enabled": true, "redirectUris": ["'${KEYCLOAK_REDIRECT_URI}'"]}') + -d '{"clientId": "'${CLIENT}'", + "publicClient": true, + "defaultClientScopes": ["pbench", "openid", "profile", "email"], + "directAccessGrantsEnabled": true, + "serviceAccountsEnabled": true, + "enabled": true, + "redirectUris": ["'${KEYCLOAK_REDIRECT_URI}'"]}') CLIENT_ID=$(grep -o -e 'http://[^[:space:]]*' <<< ${CLIENT_CONF} | sed -e 's|.*/||') if [[ -z "${CLIENT_ID}" ]]; then diff --git a/server/pbenchinacan/run-pbench-in-a-can b/server/pbenchinacan/run-pbench-in-a-can index 3a8587e7fb..14ea49054c 100755 --- a/server/pbenchinacan/run-pbench-in-a-can +++ b/server/pbenchinacan/run-pbench-in-a-can @@ -24,7 +24,7 @@ export PB_SERVER_IMAGE_PULL_POLICY="${PB_SERVER_IMAGE_PULL_POLICY:-${PB_COMMON_I export PB_DASHBOARD_DIR="${PB_DASHBOARD_DIR:-${PWD}/dashboard/build/}" export KEYCLOAK_REALM=${KEYCLOAK_REALM:-"pbench-server"} -export KEYCLOAK_CLIENT=${KEYCLOAK_CLIENT:-"pbench-dashboard"} +export KEYCLOAK_CLIENT=${KEYCLOAK_CLIENT:-"pbench-client"} # Set up TMP_DIR, if it's not already defined, to point to WORKSPACE_TMP, if it # is defined (e.g., by the CI), or to `/var/tmp/pbench` as a fallback.