[AAP-63312] Add workload_ttl_seconds field to WorkloadIdentityTokenRequestSerializer#952
[AAP-63312] Add workload_ttl_seconds field to WorkloadIdentityTokenRequestSerializer#952arrestle wants to merge 9 commits intoansible:develfrom
Conversation
Assisted-by Claude(4.5-sonnet)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a hard cap constant and an optional, nullable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
arrestle
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible_base/lib/workload_identity/workload_identity_tokens.py`:
- Around line 28-38: The IntegerField workload_ttl_seconds currently has no
upper bound; add a max_value to the serializers.IntegerField (e.g.,
max_value=settings.JWT_MAX_TTL_SECONDS or max_value=jwt_default_ttl_seconds) to
prevent arbitrarily large TTLs, update the help_text to document the maximum,
and ensure any referenced constant (JWT_MAX_TTL_SECONDS/jwt_default_ttl_seconds)
is imported or available where workload_ttl_seconds is defined so validation
uses the intended platform maximum.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
ansible_base/lib/workload_identity/workload_identity_tokens.pytest_app/tests/lib/workload_identity/test_serializers.py
john-westcott-iv
left a comment
john-westcott-iv
left a comment
There was a problem hiding this comment.
Code Review Summary
Clean serializer addition — the field definition is well-structured with proper optionality and the test coverage hits all the boundary cases (valid, zero, null, omitted, negative, non-integer).
Files Reviewed: 2 files
Comments Posted: 1 reply to existing thread
Issues Found
- 1 security suggestion (no
max_value— reinforced existing thread with additional context) - 0 correctness/logic issues
- 0 code quality suggestions
Overall Assessment
LGTM with the max_value addition. Replied to the existing thread with +1 and rationale — a configurable max conforms to "no hard maximum" while preventing effectively infinite token lifetimes (a known complaint from our OAuth token experience).
General Feedback
- Test assertions on error codes (
min_value,invalid) are a nice touch allow_null=True+required=Falsecombination correctly handles both omission and explicit null
dleehr
left a comment
dleehr
left a comment
There was a problem hiding this comment.
Nice use of the serializer min_value to let validation handle this. Requesting changes around the special-casing of 0 (also noted in the other PR) and pending a decision on the max_value
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test_app/tests/lib/workload_identity/test_serializers.py (1)
222-323: Consider adding exact boundary pass-tests formin_value=1andmax_value=WORKLOAD_TTL_MAX_SECONDS.The suite confirms that
0andmax+1fail, but doesn't verify that1andWORKLOAD_TTL_MAX_SECONDSthemselves are accepted. A future change to the constraints would silently break those boundaries without a test catching it.➕ Suggested additions
def test_workload_ttl_seconds_min_boundary_passes(self): data = { 'scope': 'openid', 'audience': 'https://api.example.com', 'claims': {'job_name': 'test-job'}, 'workload_ttl_seconds': 1, } serializer = WorkloadIdentityTokenRequestSerializer(data=data) assert serializer.is_valid(), f"Serializer errors: {serializer.errors}" assert serializer.validated_data['workload_ttl_seconds'] == 1 def test_workload_ttl_seconds_max_boundary_passes(self): data = { 'scope': 'openid', 'audience': 'https://api.example.com', 'claims': {'job_name': 'test-job'}, 'workload_ttl_seconds': WORKLOAD_TTL_MAX_SECONDS, } serializer = WorkloadIdentityTokenRequestSerializer(data=data) assert serializer.is_valid(), f"Serializer errors: {serializer.errors}" assert serializer.validated_data['workload_ttl_seconds'] == WORKLOAD_TTL_MAX_SECONDS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_app/tests/lib/workload_identity/test_serializers.py` around lines 222 - 323, Add two boundary tests to assert the inclusive min and max are accepted: implement test_workload_ttl_seconds_min_boundary_passes and test_workload_ttl_seconds_max_boundary_passes that each construct a payload with workload_ttl_seconds set to 1 and WORKLOAD_TTL_MAX_SECONDS respectively, instantiate WorkloadIdentityTokenRequestSerializer with that data, assert serializer.is_valid() and that serializer.validated_data['workload_ttl_seconds'] equals the provided value; place them alongside the existing workload_ttl_seconds tests so future changes to the min_value=1 and max_value=WORKLOAD_TTL_MAX_SECONDS constraints are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible_base/lib/workload_identity/workload_identity_tokens.py`:
- Around line 42-47: The help_text uses an EN DASH and an f-string inside the
gettext_lazy call which breaks extraction; change the en-dash to a plain hyphen
and remove the f-string by making the translatable string a plain literal (e.g.
assign a template string constant like HELP_TEXT_TTL = "Optional
workload-specific TTL override in seconds (1-{max}). If provided, ..." and then
call _(HELP_TEXT_TTL).format(max=WORKLOAD_TTL_MAX_SECONDS)) so that _('...')
contains no f-string; update the help_text assignment that references
WORKLOAD_TTL_MAX_SECONDS and use the .format(...) approach (or inline the
numeric constant) to keep gettext_extractable.
---
Nitpick comments:
In `@test_app/tests/lib/workload_identity/test_serializers.py`:
- Around line 222-323: Add two boundary tests to assert the inclusive min and
max are accepted: implement test_workload_ttl_seconds_min_boundary_passes and
test_workload_ttl_seconds_max_boundary_passes that each construct a payload with
workload_ttl_seconds set to 1 and WORKLOAD_TTL_MAX_SECONDS respectively,
instantiate WorkloadIdentityTokenRequestSerializer with that data, assert
serializer.is_valid() and that serializer.validated_data['workload_ttl_seconds']
equals the provided value; place them alongside the existing
workload_ttl_seconds tests so future changes to the min_value=1 and
max_value=WORKLOAD_TTL_MAX_SECONDS constraints are caught.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
ansible_base/lib/workload_identity/workload_identity_tokens.pytest_app/tests/lib/workload_identity/test_serializers.py
dleehr
left a comment
dleehr
left a comment
There was a problem hiding this comment.
Looks good, but the terraform-related comments lack context and may be confusing. Please remove those.
Addressed dleehr review: external vendor references add no value in this context and may be confusing. Made-with: Cursor
john-westcott-iv
left a comment
john-westcott-iv
left a comment
There was a problem hiding this comment.
Code Review Summary
Clean serializer addition with all prior review feedback addressed. The field definition, optionality handling (allow_null=True + required=False), and test coverage are solid.
Files Reviewed: 2 files
Comments Posted: 3 inline suggestions
Issues Found
- 0 security/correctness issues
- 3 minor quality suggestions (all non-blocking)
Overall Assessment
LGTM — approving with minor suggestions:
- Consider using
get_setting()formax_valueto enable per-service configurability (established DAB pattern) - Drop the hardcoded range
(1-86400)fromhelp_textto match DAB conventions and avoid staleness - Parameterize the invalid-value tests and add a float case per @dleehr and @dragid10's discussion
Note: This review was submitted with assistance from Claude AI assistant on behalf of John Westcott.
Co-authored-by: John Westcott IV <32551173+john-westcott-iv@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible_base/lib/workload_identity/workload_identity_tokens.py`:
- Around line 33-37: The code references get_setting in the workload_ttl_seconds
field but does not import it, causing a NameError at import time; add an import
for get_setting at the top of the module (e.g. from ansible_base.settings import
get_setting) so workload_ttl_seconds can call get_setting(...,
WORKLOAD_TTL_MAX_SECONDS) without failing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
ansible_base/lib/workload_identity/workload_identity_tokens.py
davemulford
left a comment
davemulford
left a comment
There was a problem hiding this comment.
Changes look good. I see there is a failing test but that's happening outside of the changes you've made, and you're following up with the django-ansible-base team about that.
…TL assisted-by: Claude
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
|
dleehr
left a comment
dleehr
left a comment
There was a problem hiding this comment.
I'm requesting changes to block this merge for now.
The most recent commits modify the validation logic that was approved and are now using custom logic and a previously-unused class. I need to understand that better before merging.
The validation logic changed because max_value=get_setting(...) on the field was evaluated at class definition time and caused pure-python-imports to fail (Django settings aren’t available at import time). |
6 participants
[AAP-63312] Add workload_ttl_seconds field to WorkloadIdentityTokenRequestSerializer
Description
What is being changed?
Adding optional
workload_ttl_secondsparameter toWorkloadIdentityTokenRequestSerializerto support workload-specific JWT TTL overrides.Why is this change needed?
Gateway's AAP-63312 implementation requires the ability to accept workload-specific TTL values from API requests. This serializer change enables the API contract for that feature.
How does this change address the issue?
Adds a new optional
IntegerFieldto the request serializer that validates TTL override values while maintaining backward compatibility (field is optional).Type of Change
Self-Review Checklist
Implementation
Modified:
ansible_base/lib/workload_identity/workload_identity_tokens.py(11 lines)Modified:
test_app/tests/lib/workload_identity/test_serializers.py(86 lines)Added optional
workload_ttl_secondsfield with validation (min_value=0, allow_null, not required).Testing Instructions
# Run serializer tests tox -e py -- test_app/tests/lib/workload_identity/test_serializers.py -vExpected: 29 tests pass (22 existing + 7 new)
Additional Context
Backward compatible: Field is optional, existing requests work unchanged.
Requires coordinated Gateway PR to implement TTL calculation logic.
Related: AAP-63296 (P4.1), ANSTRAT-1019
Development Note: This implementation was developed with assistance from Claude AI assistant.
https://handbook.eng.ansible.com/proposals/ANSTRAT-1019-Configurable-JWT-Expiration-for-OIDC-Integrations
Assisted-by: Claude
Summary by CodeRabbit
New Features
Tests