feat: add activation instance scoping to JWT tokens for WebSocket aut…#1602
Open
mkanoor wants to merge 1 commit into
Open
feat: add activation instance scoping to JWT tokens for WebSocket aut…#1602mkanoor wants to merge 1 commit into
mkanoor wants to merge 1 commit into
Conversation
|
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:
📝 WalkthroughWalkthroughJWT tokens now optionally carry ChangesJWT Activation Instance Scoping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/api/test_auth_serializers.py (1)
91-101: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake the expired-token test deterministic without wall-clock sleep.
Line 100 adds
time.sleep(1), which slows the suite and can still be timing-sensitive. Patch to an already-expired lifetime (e.g.,-1) and remove the sleep.Proposed change
with patch( "aap_eda.services.auth.settings.JWT_REFRESH_TOKEN_LIFETIME_DAYS", - "0", + "-1", ): _, refresh_token = create_jwt_token() - - import time - - time.sleep(1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/api/test_auth_serializers.py` around lines 91 - 101, The test for expired tokens uses time.sleep(1) to wait for expiration, which is slow and timing-sensitive. Remove the import and time.sleep(1) call entirely, and change the patched value for JWT_REFRESH_TOKEN_LIFETIME_DAYS from "0" to "-1" to create a token that is already expired by definition rather than relying on wall-clock time to pass. This makes the test deterministic and eliminates the unnecessary delay.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration/api/test_auth_serializers.py`:
- Around line 91-101: The test for expired tokens uses time.sleep(1) to wait for
expiration, which is slow and timing-sensitive. Remove the import and
time.sleep(1) call entirely, and change the patched value for
JWT_REFRESH_TOKEN_LIFETIME_DAYS from "0" to "-1" to create a token that is
already expired by definition rather than relying on wall-clock time to pass.
This makes the test deterministic and eliminates the unnecessary delay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 96d67dcf-8229-481f-a7d0-16ae7940397e
📒 Files selected for processing (10)
src/aap_eda/api/serializers/auth.pysrc/aap_eda/api/views/auth.pysrc/aap_eda/services/activation/engine/common.pysrc/aap_eda/services/auth.pysrc/aap_eda/wsapi/consumers.pytests/integration/api/test_auth.pytests/integration/api/test_auth_serializers.pytests/integration/services/test_auth.pytests/integration/wsapi/test_consumer.pytests/integration/wsapi/test_consumer_token_validation.py
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1602 +/- ##
==========================================
+ Coverage 92.48% 92.53% +0.05%
==========================================
Files 244 244
Lines 11244 11333 +89
==========================================
+ Hits 10399 10487 +88
- Misses 845 846 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
mkanoor
force-pushed
the
activation_based_token
branch
4 times, most recently
from
4a345f0 to
fa7a5b1
Compare
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aap_eda/wsapi/consumers.py`:
- Around line 241-243: In the InvalidTokenError exception handler, the
logger.error call is logging the entire inbound message payload via the `data`
variable, which can leak sensitive information. Replace the full `data` object
in the log message with minimal identifying information needed for debugging
purposes, such as message type or a request ID if available. Keep the error
details from the `err` variable in the log as those are essential for
troubleshooting token authentication failures.
- Around line 131-132: The auth_header assignment at line 131 uses direct
decode() on potentially malformed bytes which can raise UnicodeDecodeError, and
this exception is not caught by the InvalidTokenError handler. Fix this by
wrapping the decode call in a try-except block to catch UnicodeDecodeError, or
use the decode method's error handling parameter (such as errors='replace' or
errors='ignore') to safely handle malformed UTF-8 bytes. Ensure that any decode
failures are treated as authentication failures that follow the intended 4003
auth failure path rather than propagating as uncaught exceptions.
In `@tests/integration/wsapi/test_consumer.py`:
- Around line 167-168: The disconnect calls on ws_communicator are
inconsistently handling asyncio.CancelledError across different test methods.
Wrap all instances of `await ws_communicator.disconnect()` with a try/except
block that catches `asyncio.CancelledError` and passes silently, applying this
pattern uniformly across all test methods including those at lines 167-168,
211-212, 1425-1426, 1467-1468, 1788-1789, 1821-1822, and 1969-1970 to prevent
intermittent test failures in Django 5.x.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: ce8dbec4-7dbe-45fc-9978-df325bec513e
📒 Files selected for processing (10)
src/aap_eda/api/serializers/auth.pysrc/aap_eda/api/views/auth.pysrc/aap_eda/services/activation/engine/common.pysrc/aap_eda/services/auth.pysrc/aap_eda/wsapi/consumers.pytests/integration/api/test_auth.pytests/integration/api/test_auth_serializers.pytests/integration/services/test_auth.pytests/integration/wsapi/test_consumer.pytests/integration/wsapi/test_consumer_token_validation.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/aap_eda/services/activation/engine/common.py
- src/aap_eda/api/serializers/auth.py
- src/aap_eda/api/views/auth.py
- tests/integration/api/test_auth_serializers.py
mkanoor
force-pushed
the
activation_based_token
branch
2 times, most recently
from
838dacb to
8e65325
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aap_eda/wsapi/consumers.py`:
- Around line 216-220: The scope validation logic currently only checks for
activation_id in the incoming message data, but the code at line 502 also uses
message.ansible_rulebook_id as an activation instance for job processing. Update
the validation block to handle both cases: first check if activation_id exists
in the data, and if not, check for message.ansible_rulebook_id. Pass whichever
activation identifier is present to the _validate_token_scope() method, ensuring
that scope validation is applied regardless of which field contains the
activation reference. Additionally, add a regression test case that verifies
_validate_token_scope() is called when a JOB payload contains
ansible_rulebook_id but lacks activation_id.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 44e1643c-fb90-464c-a26b-2495063ab3ff
📒 Files selected for processing (10)
src/aap_eda/api/serializers/auth.pysrc/aap_eda/api/views/auth.pysrc/aap_eda/services/activation/engine/common.pysrc/aap_eda/services/auth.pysrc/aap_eda/wsapi/consumers.pytests/integration/api/test_auth.pytests/integration/api/test_auth_serializers.pytests/integration/services/test_auth.pytests/integration/wsapi/test_consumer.pytests/integration/wsapi/test_consumer_token_validation.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/aap_eda/api/views/auth.py
- src/aap_eda/services/activation/engine/common.py
- src/aap_eda/api/serializers/auth.py
- tests/integration/api/test_auth_serializers.py
- tests/integration/wsapi/test_consumer.py
mkanoor
force-pushed
the
activation_based_token
branch
from
8e65325 to
73f96bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aap_eda/wsapi/consumers.py`:
- Around line 145-147: WebSocket authentication in the consumer currently
accepts any JWT that passes parse_jwt_token(), so refresh tokens can slip
through if the scope check matches. Update the token handling in the consumer
method that sets _token_payload to explicitly require an access token by
checking the token_type claim after parse_jwt_token() returns, and reject the
connection otherwise. Use the existing token parsing and scope validation flow
in the WebSocket consumer to enforce this before returning the payload.
- Around line 221-227: The JOB scope check currently validates against
activation_id even when the record is persisted using ansible_rulebook_id, which
can let a token pass for one ID and write to another activation instance. Update
the validation in the message handling path around _validate_token_scope so JOB
messages always scope-check the same identifier used later for persistence,
preferring ansible_rulebook_id for MessageType.JOB and falling back only when
that value is unavailable.
In `@tests/integration/wsapi/test_consumer_token_validation.py`:
- Around line 362-393: The JOB token-validation tests in the consumer suite only
assert that the socket closes, so tighten them by patching the dispatch path and
checking whether the message handler ran. In the rejection cases around
test_job_message_rejects_mismatched_ansible_rulebook_id and the related receive
tests, mock handle_jobs on the consumer and assert it is not called when auth
fails; in the precedence test, assert it is called exactly once when the
higher-priority token source wins. Keep the existing close assertions, but add
handler-call assertions so the tests verify both blocking and dispatch behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 85f897ca-f03b-41e3-9e08-92f184a91bf0
📒 Files selected for processing (10)
src/aap_eda/api/serializers/auth.pysrc/aap_eda/api/views/auth.pysrc/aap_eda/services/activation/engine/common.pysrc/aap_eda/services/auth.pysrc/aap_eda/wsapi/consumers.pytests/integration/api/test_auth.pytests/integration/api/test_auth_serializers.pytests/integration/services/test_auth.pytests/integration/wsapi/test_consumer.pytests/integration/wsapi/test_consumer_token_validation.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/aap_eda/api/views/auth.py
- src/aap_eda/api/serializers/auth.py
- src/aap_eda/services/activation/engine/common.py
- tests/integration/api/test_auth_serializers.py
- tests/integration/wsapi/test_consumer.py
mkanoor
force-pushed
the
activation_based_token
branch
2 times, most recently
from
0fd6c35 to
559e619
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aap_eda/wsapi/consumers.py (1)
306-313: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate scope before DB-backed log tracking.
Line 306 uses untrusted
activation_idbefore token scope validation and outside thetry, so invalid scoped messages can trigger DB lookups or uncaughtObjectDoesNotExistbefore the intended 4003 auth path.Proposed fix
- await self._set_log_tracking_id(data) - logger.debug(f"AnsibleRulebookConsumer received: {data}") msg_type = MessageType(data.get("type")) try: await self._validate_message_token_scope(msg_type, data) + await self._set_log_tracking_id(data) await self._dispatch_message(msg_type, data)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/wsapi/consumers.py` around lines 306 - 313, The message handling in AnsibleRulebookConsumer should validate token scope before any DB-backed log tracking that uses activation_id. Move the _validate_message_token_scope call ahead of _set_log_tracking_id in the receive flow, and keep both inside the existing try path so invalid scoped messages are rejected with the intended 4003 auth handling instead of triggering uncaught ObjectDoesNotExist from _set_log_tracking_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aap_eda/wsapi/consumers.py`:
- Around line 221-258: The current _validate_message_token_scope flow skips
ANSIBLE_EVENT because AnsibleEventMessage lacks activation_id, but
_dispatch_message still processes it and insert_event_related_data persists by
event.job_id. Update _validate_message_token_scope (and the related
_dispatch_message/insert_event_related_data path) to resolve the activation from
job_id for MessageType.ANSIBLE_EVENT and validate that scope, or explicitly
reject ANSIBLE_EVENT here if that message type should no longer be accepted.
- Around line 145-156: The WebSocket auth flow in _get_token_payload currently
caches the parsed JWT payload before validating token_type, which allows
rejected refresh-token payloads to be reused on later calls. Update
_get_token_payload in consumers.py so self._token_payload is assigned only after
the token_type check passes and the token is confirmed to be an access token;
keep the validation and caching together so only validated access-token payloads
are stored.
---
Outside diff comments:
In `@src/aap_eda/wsapi/consumers.py`:
- Around line 306-313: The message handling in AnsibleRulebookConsumer should
validate token scope before any DB-backed log tracking that uses activation_id.
Move the _validate_message_token_scope call ahead of _set_log_tracking_id in the
receive flow, and keep both inside the existing try path so invalid scoped
messages are rejected with the intended 4003 auth handling instead of triggering
uncaught ObjectDoesNotExist from _set_log_tracking_id.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 8629ac97-90d6-47f2-820c-2fc36c83665d
📒 Files selected for processing (10)
src/aap_eda/api/serializers/auth.pysrc/aap_eda/api/views/auth.pysrc/aap_eda/services/activation/engine/common.pysrc/aap_eda/services/auth.pysrc/aap_eda/wsapi/consumers.pytests/integration/api/test_auth.pytests/integration/api/test_auth_serializers.pytests/integration/services/test_auth.pytests/integration/wsapi/test_consumer.pytests/integration/wsapi/test_consumer_token_validation.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/aap_eda/api/serializers/auth.py
- src/aap_eda/api/views/auth.py
- src/aap_eda/services/activation/engine/common.py
- tests/integration/api/test_auth_serializers.py
- tests/integration/wsapi/test_consumer.py
mkanoor
force-pushed
the
activation_based_token
branch
from
559e619 to
798f292
Compare
…hentication This commit adds security enhancements to ensure JWT tokens are scoped to specific activation instances, preventing token reuse across different instances. It also implements secure token refresh with scope preservation and backward- compatible legacy token support with actionable warnings. Key changes: - Add activation_instance_id to JWT token payload (supports int and UUID) - Validate token scope in WebSocket consumer before processing messages - Implement secure token refresh that preserves activation_instance_id scope - Support backward compatibility with legacy tokens (no forced logout) - Add actionable warnings for legacy tokens with activation name - Implement token payload caching for performance Security improvements: - Tokens cannot be used for different activation instances - Token refresh preserves scope (prevents scope escalation vulnerability) - Two-layer validation: middleware (signature/expiry) + consumer (scope) - Automatic revocation via database existence checks - Comprehensive error logging for audit trails Token Refresh Security: - RefreshTokenSerializer extracts activation_instance_id from refresh token - New access tokens preserve the scope from refresh token (trusted source) - Prevents privilege escalation by not accepting client-provided scope - Legacy refresh tokens continue to work (no scope added) Backward Compatibility: - Legacy tokens without activation_instance_id are accepted with warnings - Warnings logged once per connection to avoid log spam - Warnings include activation name for easy operator action - No event loss, no forced disconnection Security Fixes: - Fix UnicodeDecodeError handling for malformed Authorization headers - Prevent sensitive data leakage in error logs (only log msg_type and activation_id) - Close token scope validation bypass for JOB messages using ansible_rulebook_id - Add regression tests for all security fixes Files modified: - src/aap_eda/services/auth.py: Token creation and validation - src/aap_eda/services/activation/engine/common.py: Pass activation_instance_id - src/aap_eda/wsapi/consumers.py: Token scope validation with async warnings - src/aap_eda/api/serializers/auth.py: Extract scope from refresh tokens - src/aap_eda/api/views/auth.py: Preserve scope in token refresh endpoint Tests added: - 9 tests for auth service (token creation, validation, scope checking) - 14 tests for consumer validation (scope matching, legacy token warnings) - 4 tests for security fixes (malformed headers, log sanitization, JOB validation) - 3 tests for token refresh endpoint (scope preservation, legacy handling) - 7 tests for RefreshTokenSerializer (scope extraction, error handling) - Total: 37 passing tests All code formatted with black, sorted with isort, and linted with flake8. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
mkanoor
force-pushed
the
activation_based_token
branch
from
798f292 to
3d4f482
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WebSocket Authentication
This commit adds security enhancements to ensure JWT tokens are scoped to specific activation instances, preventing token reuse across different instances. It also implements secure token refresh with scope preservation and backward- compatible legacy token support with actionable warnings.
Key changes:
Security improvements:
Token Refresh Security:
Backward Compatibility:
Files modified:
Tests added:
All code formatted with black, sorted with isort, and linted with flake8.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests