Conversation
ajzobro
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1029 +/- ##
==========================================
- Coverage 77.21% 77.18% -0.04%
==========================================
Files 116 118 +2
Lines 12065 12114 +49
Branches 996 997 +1
==========================================
+ Hits 9316 9350 +34
- Misses 2520 2535 +15
Partials 229 229
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces OWASP-style security/audit logging into the Testflinger server, wiring OWASP log events into authentication/authorization flows and client-permission management, and adds tests to validate key auth-related events.
Changes:
- Add
owasp-logger(and related dependencies) and regenerateuv.lock. - Introduce a
testflinger.owasp.OWASPLoggerwrapper and attach an OWASP logger instance to the Flask app for use across request handlers. - Emit OWASP audit events for authn/authz and client permission create/update/delete, plus add unit tests for core authn/authz logging.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| server/pyproject.toml | Adds owasp-logger dependency (currently with otel extra). |
| server/uv.lock | Lockfile updates for new deps; includes some dependency downgrades. |
| server/src/testflinger/owasp/init.py | Adds OWASP logger wrapper + request metadata helper + OTELResource stub. |
| server/src/testflinger/owasp/views.py | Adds stub OWASP views module (no active endpoints yet). |
| server/src/testflinger/application.py | Attaches OWASP logger to the Flask app and uses it for exception handlers. |
| server/src/testflinger/api/auth.py | Logs authz failures and normalizes role values to ServerRoles. |
| server/src/testflinger/api/v1.py | Logs authn events on token issuance/revocation and user management events. |
| server/src/testflinger/api/schemas.py | Updates Marshmallow boolean default handling for NoProvisionData. |
| server/src/testflinger/secrets/init.py | Wraps secrets-store logger with OWASP logger. |
| server/tests/test_owasp_logging.py | New tests validating OWASP authn/authz logging events. |
| common/src/testflinger_common/enums.py | Changes ServerRoles.__str__ to return the raw value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Log token revocation | ||
| target_client_id = token_entry.get("client_id", "unknown") | ||
| current_app.owasp_logger.authn_token_revoked( | ||
| userid=target_client_id, | ||
| tokenid=token[:16] + "...", # Log first 16 chars of token for tracking |
There was a problem hiding this comment.
authn_token_revoked logs a portion of the refresh token (token[:16] + '...'). Even partial tokens are sensitive credentials and shouldn’t be written to logs. Prefer logging a non-sensitive identifier (e.g., a hash/fingerprint of the token, the DB document id, or just the client_id) to allow correlation without credential exposure.
| # Log token revocation | |
| target_client_id = token_entry.get("client_id", "unknown") | |
| current_app.owasp_logger.authn_token_revoked( | |
| userid=target_client_id, | |
| tokenid=token[:16] + "...", # Log first 16 chars of token for tracking | |
| # Log token revocation without exposing token material | |
| target_client_id = token_entry.get("client_id", "unknown") | |
| token_log_id = str(token_entry.get("_id") or target_client_id) | |
| current_app.owasp_logger.authn_token_revoked( | |
| userid=target_client_id, | |
| tokenid=token_log_id, |
| def test_authn_token_revoked(self, mongo_app_with_permissions, caplog): | ||
| """Verify authn_token_revoked is logged when refresh token | ||
| is revoked. | ||
| """ | ||
| import os | ||
|
|
||
| os.environ["JWT_SIGNING_KEY"] = "my_secret_key" |
There was a problem hiding this comment.
This test does an in-function import os and then sets os.environ["JWT_SIGNING_KEY"] directly. Consider removing the redundant import and using monkeypatch.setenv(...) so the env var is reliably restored even if the test fails partway through.
| def test_authn_token_revoked(self, mongo_app_with_permissions, caplog): | |
| """Verify authn_token_revoked is logged when refresh token | |
| is revoked. | |
| """ | |
| import os | |
| os.environ["JWT_SIGNING_KEY"] = "my_secret_key" | |
| def test_authn_token_revoked( | |
| self, mongo_app_with_permissions, caplog, monkeypatch | |
| ): | |
| """Verify authn_token_revoked is logged when refresh token | |
| is revoked. | |
| """ | |
| monkeypatch.setenv("JWT_SIGNING_KEY", "my_secret_key") |
| "gunicorn>=23.0.0", | ||
| "gevent>=24.2.1", | ||
| "hvac>=2.3.0", | ||
| "owasp-logger[otel]>=0.1.5", |
There was a problem hiding this comment.
The dependency is added as owasp-logger[otel], which pulls in OpenTelemetry SDK/instrumentation dependencies, but this PR’s code doesn’t appear to use OTEL yet (and the testflinger.owasp module describes OTEL as future work). Consider depending on owasp-logger without the otel extra for now, or making the OTEL extra optional via a project extra to avoid increasing the default dependency footprint.
| "owasp-logger[otel]>=0.1.5", | |
| "owasp-logger>=0.1.5", |
| role = permissions.get("role", ServerRoles.CONTRIBUTOR) | ||
| # Convert role string to enum if needed | ||
| if isinstance(role, str): | ||
| g.role = ServerRoles(role) |
There was a problem hiding this comment.
Converting a stored role string via ServerRoles(role) can raise ValueError if the DB contains an unexpected value (e.g. legacy data/corruption), turning an auth failure into a 500. Wrap this conversion in try/except and fall back to a safe default (or abort with 403) when the role isn’t recognized.
| g.role = ServerRoles(role) | |
| try: | |
| g.role = ServerRoles(role) | |
| except ValueError: | |
| current_app.owasp_logger.authz_fail( | |
| userid=permissions.get("client_id", "unknown"), | |
| resource=request.path, | |
| description=( | |
| f"Authorization denied: client " | |
| f"{permissions.get('client_id', 'unknown')} " | |
| f"provided unrecognized role {role!r}" | |
| ), | |
| **OWASPLogger.get_request_metadata(request), | |
| ) | |
| abort( | |
| HTTPStatus.FORBIDDEN, | |
| "Authentication contains an unrecognized role", | |
| ) |
| """Initialize OTEL resource from Flask context.""" | ||
| try: | ||
| version = importlib.metadata.version("testflinger") | ||
| except importlib.metadata.PackageNotFoundError: | ||
| version = "devel" |
There was a problem hiding this comment.
OTELResource references importlib.metadata but this module only does import importlib. Submodules aren’t imported automatically, so importlib.metadata will raise AttributeError when executed. Import importlib.metadata explicitly (or from importlib import metadata) and update these references.
| """Schema for the `provision_data` section of a no-provision job.""" | ||
|
|
||
| skip = fields.Boolean(required=False, default=True) | ||
| skip = fields.Boolean(required=False, dump_default=True) |
There was a problem hiding this comment.
NoProvisionData.skip previously defaulted to True when loading request payloads. Switching to dump_default=True only affects serialization; it won’t apply a default on input deserialization. Use load_default=True (and optionally keep dump_default=True) to preserve the prior behavior for API requests.
| skip = fields.Boolean(required=False, dump_default=True) | |
| skip = fields.Boolean(required=False, load_default=True, dump_default=True) |
| description=( | ||
| f"Admin {g.client_id} updated permissions for " | ||
| f"client {client_id}: role={new_role}" | ||
| ), |
There was a problem hiding this comment.
user_updated / user_created logs interpolate new_role, but new_role can be None when the request didn’t include a role change. This can make audit logs misleading (e.g. “role=None”). Consider logging the effective role (new if provided else current) and/or only including fields actually changed.
| current_app.owasp_logger.user_updated( | ||
| userid=g.client_id, | ||
| onuserid=client_id, |
There was a problem hiding this comment.
New OWASP audit events are emitted for client permission create/update/delete (user_created, user_updated, user_deleted), but the new/updated tests only cover authn/authz events. Since logging is the main feature here, consider adding assertions in existing client-permissions endpoint tests to verify these events are logged as expected.
| self, endpoint, method, test_role, mongo_app, caplog | ||
| ): | ||
| """ | ||
| Verify authz_fail is logged when authenticated user with | ||
| insufficient role attempts to access protected endpoint. | ||
| """ | ||
| os.environ["JWT_SIGNING_KEY"] = "my_secret_key" |
There was a problem hiding this comment.
This test mutates os.environ["JWT_SIGNING_KEY"] directly, which can leak state to other tests if an assertion fails. Prefer using the monkeypatch fixture (or a context manager) so the environment change is automatically reverted.
| self, endpoint, method, test_role, mongo_app, caplog | |
| ): | |
| """ | |
| Verify authz_fail is logged when authenticated user with | |
| insufficient role attempts to access protected endpoint. | |
| """ | |
| os.environ["JWT_SIGNING_KEY"] = "my_secret_key" | |
| self, | |
| endpoint, | |
| method, | |
| test_role, | |
| mongo_app, | |
| caplog, | |
| monkeypatch, | |
| ): | |
| """ | |
| Verify authz_fail is logged when authenticated user with | |
| insufficient role attempts to access protected endpoint. | |
| """ | |
| monkeypatch.setenv("JWT_SIGNING_KEY", "my_secret_key") |
| """Schema for the `provision_data` section of a no-provision job.""" | ||
|
|
||
| skip = fields.Boolean(required=False, default=True) | ||
| skip = fields.Boolean(required=False, dump_default=True) |
There was a problem hiding this comment.
@rene-oromtz this is due to the marshmallow version
Description
Resolved issues
Documentation
Web service API changes
Tests