Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generation of API key on pbench-server #3368

Merged
merged 14 commits into from
Apr 14, 2023
Merged

Generation of API key on pbench-server #3368

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
e6bf449
Rework api_key model and implement _key functionality .
Mar 2, 2023
7b45b07
Remove unused fixtures and comment fixes and code cleanups
Apr 5, 2023
d3f29ca
Remove auth_tokens.py and updated alembic file
Apr 5, 2023
e4e1bf2
Fix black check - remove unused imports
Apr 5, 2023
e2a9217
Rebased and fix merge conflicts
Apr 9, 2023
5b5ef6f
Fix alembic error of circular imports
Apr 9, 2023
797081b
Addressed review comments and removed audience tag from payload
Apr 11, 2023
3306278
Updated alembic file
Apr 11, 2023
798d22e
Updated the duplicate key check & did assert of audit
Apr 12, 2023
5c64bcd
Fix black error and simplified return statement
Apr 12, 2023
71a12e0
Review comments and generalize decode_integrity_error
Apr 12, 2023
02bab01
Modified status code to OK for the DuplicateApiKey and review comments
Apr 13, 2023
7d3de88
Minor code refactoring
Apr 13, 2023
831ad0b
Updated type of on_null and on_duplicate as Callable
Apr 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/pbench/server/api/resources/api_key.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ def _post(
key = APIKey(api_key=new_key, user=user)
key.add()
except DuplicateApiKey:
pass
response = jsonify({"api_key": new_key})
response.status_code = HTTPStatus.OK
return response
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a little "heavy" (or not DRY enough). I think it would be better to use a local variable to capture the response status, which you would set to OK here and to CREATED either inside the try block after line 67 or in an else clause on the try block, and then have a common exit path where you create the response, set the status_code, and return.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also skipping the audit attributes setting ...

Although ... this makes me realize that there isn't really a clear way to record the "duplicate, alternate success" status, and I'm not sure what to do about that. Probably nothing, in the context of this PR. 😦

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also skipping the audit attributes setting ...

Another excellent reason for a common return point!

I'm not sure what to do about that. Probably nothing, in the context of this PR. 😦

Yes, not in this PR.... 😏

except Exception as e:
raise APIInternalError(str(e)) from e

Expand Down
4 changes: 2 additions & 2 deletions lib/pbench/server/auth/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ def verify_auth_oidc(auth_token: str) -> Optional[User]:
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.

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.
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.

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.
Expand Down
3 changes: 1 addition & 2 deletions lib/pbench/server/database/models/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import datetime
from pyclbr import Class

from sqlalchemy import DateTime
from sqlalchemy.exc import IntegrityError
Expand Down Expand Up @@ -55,7 +54,7 @@ def process_result_value(self, value, dialect):


def decode_integrity_error(
exception: IntegrityError, on_null: type[Class], on_duplicate: type[Class]
exception: IntegrityError, on_null: type, on_duplicate: type
) -> Exception:

"""Decode a SQLAlchemy IntegrityError to look for a recognizable UNIQUE
Expand Down
7 changes: 3 additions & 4 deletions lib/pbench/server/database/models/api_keys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def __init__(self, cause: str):
self.cause = cause

def __str__(self) -> str:
return f"Missing required key: {self.cause}"
return f"Missing required value: {self.cause}"


class APIKey(Database.Base):
Expand All @@ -56,7 +56,7 @@ class APIKey(Database.Base):
user = relationship("User")

def __str__(self):
return f"key: {self.api_key}"
return f"API key {self.api_key}"

def add(self):
"""Add an api_key object to the database."""
Expand All @@ -65,7 +65,7 @@ def add(self):
Database.db_session.commit()
except Exception as e:
Database.db_session.rollback()
self.logger.error("Can't add key:{} to DB: {}", str(self), str(e))
self.logger.error("Can't add {} to DB: {}", str(self), str(e))
raise decode_integrity_error(
e, on_duplicate=DuplicateApiKey, on_null=NullKey
)
Expand All @@ -77,7 +77,6 @@ def query(key: str) -> Optional["APIKey"]:
Returns:
An APIKey object if found, otherwise None
"""
# We currently only query api_key database with given api_key
return Database.db_session.query(APIKey).filter_by(api_key=key).first()

@staticmethod
Expand Down