chore(xtest): audit logs assertions #371
Conversation
this will hopefully reduce context overhead while refactoring
vibe-coded (sonnet 4.5) utility script for quickly getting a peek at what is currently available to test in xtest
Summary of ChangesHello @dmihalcik-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive audit log assertion framework and performs a significant and beneficial refactoring of the pytest fixtures. The new framework for audit logs is robust, providing background collection, timestamp marking, and detailed assertion capabilities, which will be very valuable for testing. The refactoring of fixtures from a monolithic conftest.py into a structured fixtures package greatly improves modularity and maintainability. The addition of new documentation and helper scripts for local development is also a welcome improvement. I've identified a few minor issues, mainly related to documentation and a typo, which are detailed in the specific comments. Overall, this is an excellent pull request that significantly enhances the repository's testing infrastructure.
| - `go 1.24` (For the Go SDK, otcfctl tool, and platform services) | ||
| - `node 22` (For the JavaScript SDK) | ||
| - `python 3.12` | ||
| - `python 3.14` |
There was a problem hiding this comment.
The required Python version is listed as 3.14, which is not a released version and appears to be a typo. The latest stable version is 3.12. Please correct this to the intended version.
| - `python 3.14` | |
| - `python 3.12` |
| **Key Python Modules**: | ||
| - `conftest.py` - Pytest configuration, fixtures, and parametrization logic | ||
| - `tdfs.py` - TDF container operations, SDK wrappers, and feature detection | ||
| - `abac.py` - Attribute-based access control helpers, otdfctl wrapper |
There was a problem hiding this comment.
The description for abac.py is now outdated due to the refactoring in this PR. The OpentdfCommandLineTool has been moved to its own file, otdfctl.py. I suggest updating this line to reflect that abac.py now only contains the Pydantic models. You should also add a new line for the otdfctl.py file.
| - `abac.py` - Attribute-based access control helpers, otdfctl wrapper | |
| - `abac.py` - Pydantic models for attribute-based access control (ABAC) policies |
| import enum | ||
| import json | ||
| import logging | ||
| import os | ||
| import subprocess | ||
| import sys | ||
| import base64 | ||
| from typing import Optional | ||
|
|
||
| from pydantic import BaseModel, ConfigDict, Field | ||
|
|
||
| logger = logging.getLogger("xtest") | ||
| logging.basicConfig() | ||
| logging.getLogger().setLevel(logging.DEBUG) | ||
|
|
||
|
|
||
| class BaseModelIgnoreExtra(BaseModel): | ||
| model_config = ConfigDict(extra="ignore") |
There was a problem hiding this comment.
The removal of the OpentdfCommandLineTool from this file and moving it to xtest/otdfctl.py is a great refactoring. It improves separation of concerns by leaving abac.py to define only the data models. However, this change makes the documentation in CLAUDE.md outdated. Please see my other comment on that file.
| # Load all fixture modules | ||
| pytest_plugins = [ | ||
| "fixtures.kas", | ||
| "fixtures.attributes", | ||
| "fixtures.assertions", | ||
| "fixtures.obligations", | ||
| "fixtures.keys", | ||
| "fixtures.audit", | ||
| ] |
There was a problem hiding this comment.
Moving the fixtures into a dedicated fixtures package and loading them via pytest_plugins is an excellent refactoring. This greatly improves the organization and maintainability of the test suite's fixtures.
| def create_temp_namesapce(otdfctl: OpentdfCommandLineTool): | ||
| """Create a temporary namespace with a random name.""" | ||
| random_ns = "".join(random.choices(string.ascii_lowercase, k=8)) + ".com" | ||
| ns = otdfctl.namespace_create(random_ns) | ||
| return ns | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def temporary_namespace(otdfctl: OpentdfCommandLineTool): | ||
| """Create a temporary namespace for test attributes.""" | ||
| try: | ||
| return create_temp_namesapce(otdfctl) | ||
| except AssertionError as e: | ||
| pytest.skip(f"Failed to create temporary namespace: {e}") |
There was a problem hiding this comment.
There is a typo in the function name create_temp_namesapce. It should be create_temp_namespace. This typo is present in both the function definition and its call site within the temporary_namespace fixture.
| def create_temp_namesapce(otdfctl: OpentdfCommandLineTool): | |
| """Create a temporary namespace with a random name.""" | |
| random_ns = "".join(random.choices(string.ascii_lowercase, k=8)) + ".com" | |
| ns = otdfctl.namespace_create(random_ns) | |
| return ns | |
| @pytest.fixture(scope="module") | |
| def temporary_namespace(otdfctl: OpentdfCommandLineTool): | |
| """Create a temporary namespace for test attributes.""" | |
| try: | |
| return create_temp_namesapce(otdfctl) | |
| except AssertionError as e: | |
| pytest.skip(f"Failed to create temporary namespace: {e}") | |
| def create_temp_namespace(otdfctl: OpentdfCommandLineTool): | |
| """Create a temporary namespace with a random name.""" | |
| random_ns = "".join(random.choices(string.ascii_lowercase, k=8)) + ".com" | |
| ns = otdfctl.namespace_create(random_ns) | |
| return ns | |
| @pytest.fixture(scope="module") | |
| def temporary_namespace(otdfctl: OpentdfCommandLineTool): | |
| """Create a temporary namespace for test attributes.""" | |
| try: | |
| return create_temp_namespace(otdfctl) | |
| except AssertionError as e: | |
| pytest.skip(f"Failed to create temporary namespace: {e}") |
| def kas_registry_import_key( | ||
| self, | ||
| kas: KasEntry | str, | ||
| private_pem: str | None, |
There was a problem hiding this comment.
Making private_pem optional in kas_registry_import_key is a good improvement, making the function more flexible for cases where only a public key is being imported or is relevant.
| # Additionally check audit logs if available | ||
| if audit_logs: | ||
| for pattern in expected_patterns: | ||
| audit_logs.assert_contains(pattern, min_count=1) | ||
|
|
There was a problem hiding this comment.
The addition of audit log checks within assert_decrypt_fails_with_patterns is a great enhancement. It strengthens the assertions by not only checking the client-side error output but also verifying that the expected events are logged on the server side.
|
1 participant
No description provided.