Addressed review comments and removed audience tag from payload

lib/pbench/server/api/__init__.py

@@ -13,7 +13,7 @@
 from pbench.common.exceptions import ConfigFileNotSpecified
 from pbench.common.logger import get_pbench_logger
 from pbench.server import PbenchServerConfig
-from pbench.server.api.resources.api_key import APIKey
+from pbench.server.api.resources.api_key import APIKeyManage
 from pbench.server.api.resources.datasets_daterange import DatasetsDateRange
 from pbench.server.api.resources.datasets_inventory import DatasetsInventory
 from pbench.server.api.resources.datasets_list import DatasetsList
@@ -132,7 +132,7 @@ def register_endpoints(api: Api, app: Flask, config: PbenchServerConfig):
         resource_class_args=(config,),
     )
     api.add_resource(
-        APIKey,
+        APIKeyManage,
         f"{base_uri}/key",
         endpoint="key",
         resource_class_args=(config,),

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

@@ -1,8 +1,7 @@
 from http import HTTPStatus
 
-from flask import jsonify, make_response
+from flask import jsonify
 from flask.wrappers import Request, Response
-from sqlalchemy.exc import IntegrityError, SQLAlchemyError
 
 from pbench.server import PbenchServerConfig
 from pbench.server.api.resources import (
@@ -16,7 +15,7 @@
     ApiSchema,
 )
 import pbench.server.auth.auth as Auth
-from pbench.server.database.models.api_key import APIKeyError, APIKeys
+from pbench.server.database.models.api_keys import APIKey, APIKeyError
 from pbench.server.database.models.audit import (
     Audit,
     AuditStatus,
@@ -25,7 +24,7 @@
 )
 
 
-class APIKey(ApiBase):
+class APIKeyManage(ApiBase):
     def __init__(self, config: PbenchServerConfig):
         super().__init__(
             config,
@@ -37,7 +36,6 @@ def __init__(self, config: PbenchServerConfig):
                 authorization=ApiAuthorizationType.NONE,
             ),
         )
-        self.server_config = config
 
     def _post(
         self, params: ApiParams, request: Request, context: ApiContext
@@ -51,7 +49,7 @@ def _post(
             Accept:         application/json
 
         Returns:
-            Success: 201 with api_key and user
+            Success: 201 with api_key
 
         Raises:
             APIAbort, reporting "UNAUTHORIZED"
@@ -62,31 +60,22 @@ def _post(
         if not user:
             raise APIAbort(
                 HTTPStatus.UNAUTHORIZED,
-                "User provided access_token is invalid or expired token",
+                "User provided access_token is invalid or expired",
             )
         try:
-            new_key = APIKeys.generate_api_key(Auth, user=user)
+            new_key = APIKey.generate_api_key(user)
         except APIKeyError as e:
-            raise APIInternalError(e.message)
+            raise APIInternalError(e.message) from e
         except Exception as e:
             raise APIInternalError("Unexpected backend error") from e
 
         try:
-            key = APIKeys(
-                api_key=new_key,
-                user=user,
-            )
+            key = APIKey(api_key=new_key, user=user)
             key.add()
-        except IntegrityError:
-            raise APIInternalError("Duplicate api_key exists in the DB")
-        except SQLAlchemyError:
-            raise APIInternalError(
-                "SQLAlchemy Exception while adding an api_key in the DB"
-            )
         except APIKeyError as e:
-            raise APIInternalError(e.message)
-        except Exception:
-            raise APIInternalError("Error while adding api_key to the DB")
+            raise APIInternalError(e.message) from e
+        except Exception as e:
+            raise APIInternalError(e) from e
 
         Audit.create(
             operation=OperationCode.CREATE,
@@ -97,8 +86,6 @@ def _post(
             status=AuditStatus.SUCCESS,
         )
 
-        response_object = {
-            "api_key": new_key,
-            "username": user.username,
-        }
-        return make_response(jsonify(response_object), HTTPStatus.CREATED)
+        response = jsonify({"api_key": new_key})
+        response.status = HTTPStatus.CREATED
+        return response

lib/pbench/server/auth/auth.py

@@ -4,11 +4,10 @@
 from flask import current_app, Flask, request
 from flask_httpauth import HTTPTokenAuth
 from flask_restful import abort
-import jwt
 
 from pbench.server import PbenchServerConfig
 from pbench.server.auth import OpenIDClient, OpenIDTokenInvalid
-from pbench.server.database.models.api_key import APIKeys
+from pbench.server.database.models.api_keys import APIKey
 from pbench.server.database.models.users import User
 
 # Module private constants
@@ -123,35 +122,22 @@ def verify_auth_api_key(api_key: str) -> Optional[User]:
         None if the api_key is not valid, a `User` object when the api_key is valid.
 
     """
-    try:
-        payload = jwt.decode(
-            api_key,
-            current_app.secret_key,
-            algorithms=_TOKEN_ALG_INT,
-            options={
-                "verify_signature": True,
-                "verify_aud": True,
-            },
-        )
-
-    except jwt.InvalidSignatureError:
-        pass
-    else:
-        key = APIKeys.query(api_key)
-        if key and payload["user_id"] == key.user.id:
-            return key.user
+    key = APIKey.query(api_key)
+    if key:
+        return key.user
     return None
 
 
 def verify_auth_oidc(auth_token: str) -> Optional[User]:
-    """Verify a token provided to the Pbench server which was obtained from a
-    third party identity provider.
+    """
+    The verification will pass either if the token is from a third-party OIDC
+    identity provider or if the token is a Pbench Server API key
 
-    `auth_token` will be validated against `verify_auth_api_key` only if
-    the provided `auth_token` is not a valid `access_token`
+    The function will first attempt to validate the token as an OIDC access token
+    if that fails, it will then attempt to validate it as a Pbench Server API key
 
-    Note: Upon token introspection if we get a valid token, we import the
-    available user information from the token into our internal User database.
+    If the token is a valid access token (and not if it is an API key),
+    we will import its contents into the internal user database.
 
     Args:
         auth_token : Token to authenticate

lib/pbench/server/database/__init__.py

@@ -4,7 +4,7 @@
 of the same is required here.
 """
 from pbench.server.database.database import Database
-from pbench.server.database.models.api_key import APIKeys  # noqa F401
+from pbench.server.database.models.api_keys import APIKey  # noqa F401
 from pbench.server.database.models.audit import Audit  # noqa F401
 from pbench.server.database.models.datasets import Dataset, Metadata  # noqa F401
 from pbench.server.database.models.server_settings import ServerSetting  # noqa F401

lib/pbench/server/database/models/api_key.py → lib/pbench/server/database/models/api_keys.py

@@ -20,13 +20,11 @@ def __str__(self):
         return repr(self.message)
 
 
-class APIKeys(Database.Base):
+class APIKey(Database.Base):
     """Model for storing the API key associated with a user."""
 
     __tablename__ = "api_keys"
-    api_key = Column(
-        String(500), primary_key=True, unique=True, nullable=False, index=True
-    )
+    api_key = Column(String(500), primary_key=True)
     created = Column(TZDateTime, nullable=False, default=TZDateTime.current_time)
     # ID of the owning user
     user_id = Column(String, ForeignKey("users.id"), nullable=False)
@@ -41,18 +39,18 @@ def add(self):
             Database.db_session.commit()
         except Exception as e:
             Database.db_session.rollback()
-            self.logger.error("Can't add {} to DB: {}", str(self), str(e))
-            raise APIKeyError("Error adding api_key to db")
+            self.logger.error("Can't add api_key to DB: {}", str(e))
+            raise APIKeyError(f"Error adding api_key to db : {e}") from e
 
     @staticmethod
-    def query(key: str) -> Optional["APIKeys"]:
+    def query(key: str) -> Optional["APIKey"]:
         """Find the given api_key in the database.
 
         Returns:
             An APIKey object if found, otherwise None
         """
         # We currently only query api_key database with given api_key
-        api_key = Database.db_session.query(APIKeys).filter_by(api_key=key).first()
+        api_key = Database.db_session.query(APIKey).filter_by(api_key=key).first()
         return api_key
 
     @staticmethod
@@ -64,13 +62,14 @@ def delete(api_key: str):
         """
         dbs = Database.db_session
         try:
-            dbs.query(APIKeys).filter_by(api_key=api_key).delete()
+            dbs.query(APIKey).filter_by(api_key=api_key).delete()
             dbs.commit()
         except Exception:
             dbs.rollback()
             raise
 
-    def generate_api_key(Auth, user: Optional[User]):
+    @staticmethod
+    def generate_api_key(user: User):
         """Creates an `api_key` for the requested user
 
         Returns:
@@ -82,11 +81,10 @@ def generate_api_key(Auth, user: Optional[User]):
             "iat": current_utc,
             "user_id": user_obj["id"],
             "username": user_obj["username"],
-            "audience": Auth.oidc_client.client_id,
         }
         try:
             generated_api_key = jwt.encode(
-                payload, current_app.secret_key, algorithm=Auth._TOKEN_ALG_INT
+                payload, current_app.secret_key, algorithm="HS256"
             )
         except (
             jwt.InvalidIssuer,

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

@@ -429,12 +429,8 @@ class TestAuthModule:
     This class does not verify the setup_app() method itself as it is verified
     in other tests related to the overall Flask application setup.
 
-    It also does not attempt to verify get_current_user_id() and
-    encode_auth_token() since they are slated for removal and are covered well
-    by other parts of the unit testing for the server code.
-
-    It does, however, verify the verify_auth() method because we need
-    it to function properly until the use of an internal user is removed.
+    It verifies the verify_auth() method which covers both OIDC token verification
+    and also the Pbench server API key verification.
     """
 
     def test_get_auth_token_succ(self, monkeypatch, make_logger):
@@ -500,7 +496,7 @@ def test_verify_auth(self, make_logger, pbench_drb_token):
         with app.app_context():
             current_app.secret_key = jwt_secret
             user = Auth.verify_auth(pbench_drb_token)
-        assert str(user.id) == DRB_USER_ID
+        assert user.id == DRB_USER_ID
 
     def test_verify_auth_invalid(self, make_logger, pbench_drb_token_invalid):
         """Verify handling of an invalid (expired) token in verify_auth"""
@@ -613,27 +609,20 @@ def test_verify_auth_api_key(
         self, monkeypatch, rsa_keys, make_logger, pbench_drb_api_key
     ):
         """Verify api_key verification via Auth.verify_auth()"""
-        client_id = "us"
+
         # Mock the Connection object and generate an OpenIDClient object,
         # installing it as Auth module's OIDC client.
-        config = mock_connection(
-            monkeypatch, client_id, public_key=rsa_keys["public_key"]
-        )
+        config = mock_connection(monkeypatch, "us", public_key=rsa_keys["public_key"])
         oidc_client = OpenIDClient.construct_oidc_client(config)
         monkeypatch.setattr(Auth, "oidc_client", oidc_client)
 
         def tio_exc(token: str) -> JSON:
             raise OpenIDTokenInvalid()
 
-        app = Flask("test-verify-auth-oidc-offline-invalid")
+        app = Flask("test_verify_auth_api_key")
         app.logger = make_logger
         with app.app_context():
             monkeypatch.setattr(oidc_client, "token_introspect", tio_exc)
-
-        """Verify success path of verify_auth_api_key"""
-        app = Flask("test-verify-auth")
-        app.logger = make_logger
-        with app.app_context():
             current_app.secret_key = jwt_secret
             user = Auth.verify_auth(pbench_drb_api_key)
         assert user.id == DRB_USER_ID
@@ -644,27 +633,19 @@ def test_verify_auth_api_key_invalid(
         """Verify api_key verification via Auth.verify_auth() fails
         gracefully with an invalid token
         """
-        client_id = "us"
         # Mock the Connection object and generate an OpenIDClient object,
         # installing it as Auth module's OIDC client.
-        config = mock_connection(
-            monkeypatch, client_id, public_key=rsa_keys["public_key"]
-        )
+        config = mock_connection(monkeypatch, "us", public_key=rsa_keys["public_key"])
         oidc_client = OpenIDClient.construct_oidc_client(config)
         monkeypatch.setattr(Auth, "oidc_client", oidc_client)
 
         def tio_exc(token: str) -> JSON:
             raise OpenIDTokenInvalid()
 
-        app = Flask("test-verify-auth-oidc-offline-invalid")
+        app = Flask("test_verify_auth_api_key_invalid")
         app.logger = make_logger
         with app.app_context():
             monkeypatch.setattr(oidc_client, "token_introspect", tio_exc)
-
-        app = Flask("test-verify-auth")
-        app.logger = make_logger
-        with app.app_context():
             current_app.secret_key = jwt_secret
-
             user = Auth.verify_auth(pbench_drb_api_key_invalid)
         assert user is None

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

@@ -28,7 +28,7 @@
 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.api_key import APIKeys
+from pbench.server.database.models.api_keys import APIKey
 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
@@ -838,11 +838,7 @@ def pbench_drb_api_key(client, server_config, create_drb_user):
 @pytest.fixture()
 def pbench_drb_api_key_invalid(client, server_config, create_drb_user):
     """Invalid api_key for the 'drb' user"""
-    return generate_api_key(
-        username="drb",
-        user=create_drb_user,
-        valid=False,
-    )
+    return "pbench_drb_invalid_api_key"
 
 
 def generate_token(
@@ -1020,17 +1016,13 @@ def generate_api_key(
     if not user:
         user = User.query(username=username)
         assert user
-    offset = datetime.timedelta(minutes=10)
-    exp = current_utc + (offset if valid else -offset)
 
     payload = {
         "iat": current_utc,
-        "exp": exp,
         "user_id": user.id,
         "username": user.username,
-        "audience": Auth.oidc_client.client_id,
     }
     key = jwt.encode(payload, jwt_secret, algorithm="HS256")
-    api_key = APIKeys(api_key=key, user=user)
+    api_key = APIKey(api_key=key, user=user)
     api_key.add()
     return key