AAP-45047: Fix allow map to process grant case in create_claims by doziedotdev · Pull Request #959 · ansible/django-ansible-base

doziedotdev · 2026-03-02T19:49:17Z

Description

What is being changed?
- The create_claims() function in claims.py now correctly handles map_type=allow when the trigger fires (has_permission=True), allowing a later allow map to override an earlier deny.
Why is this change needed?
- The allow map only handled the deny case (has_permission=False). When a trigger fired and has_permission=True, the code fell through to the catch-all else branch, logging "does not know how to be processed" and never setting access_allowed = True. This broke the documented deny-all + allow-override pattern.
How does this change address the issue?
- Changes the conditional from if auth_map.map_type == 'allow' and not has_permission: to if auth_map.map_type == 'allow': and sets access_allowed = has_permission. This handles both grant and deny cases and respects map ordering (later maps override earlier ones).

Fixes: AAP-45047, AAP-45394

Type of Change

Bug fix (non-breaking change which fixes an issue)
Test update

Self-Review Checklist

I have performed a self-review of my code
I have added relevant comments to complex code sections
I have updated documentation where needed
I have considered the security impact of these changes
I have considered performance implications
I have thought about error handling and edge cases
I have tested the changes in my local environment

Testing Instructions

Prerequisites

Any authenticator (LDAP or Local Database).

Steps to Test

Create two authenticator maps on an authenticator:
- "Deny All": map_type=allow, triggers={"never": {}}, order=1
- "Allow Override": map_type=allow, triggers={"always": {}}, order=2
Attempt to log in as any user through that authenticator

Expected Results

Login succeeds. The allow override at order 2 overrides the deny at order 1.
No "does not know how to be processed" error in logs.

Before this fix

Login fails (HTTP 401)
Log: ERROR Map type allow of rule Allow Override does not know how to be processed
Log: WARNING User <username> failed an allow map and was denied access

Summary by CodeRabbit

Bug Fixes
- Corrected authentication claims handling to ensure allow-type rules consistently and directly reflect permission decisions across all scenarios.
Tests
- Added comprehensive regression tests for authentication claims behavior, including group-based and attribute-based authorization, deny-all override patterns, and edge-case interactions.

The allow map type only handled the deny case (has_permission=False). When a trigger fired and has_permission=True, the code fell through to the catch-all else branch, logging "does not know how to be processed" and never setting access_allowed back to True. This broke the documented deny-all + allow-override pattern (the AAP 2.5+ equivalent of AUTH_LDAP_REQUIRE_GROUP). Fixes: AAP-45047, AAP-45394

coderabbitai · 2026-03-02T19:49:35Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent 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

📥 Commits

Reviewing files that changed from the base of the PR and between e956f20 and 59e2bdb.

📒 Files selected for processing (2)

ansible_base/authentication/utils/claims.py
test_app/tests/authentication/utils/test_claims.py

📝 Walkthrough

Walkthrough

The pull request modifies the authentication access control logic for 'allow' type maps, changing from conditionally updating access only when permission is False to always setting access_allowed based on the map's permission result. Comprehensive regression tests are added to cover allow/deny map interactions and edge cases.

Changes

Cohort / File(s)	Summary
Authentication Claims Logic `ansible_base/authentication/utils/claims.py`	Modified access control logic for 'allow' type maps to always set access_allowed to has_permission value and mark map as understood, removing conditional dependency on lacking permission.
Authentication Claims Tests `test_app/tests/authentication/utils/test_claims.py`	Added regression test cases covering allow/deny map scenarios: allow grant without errors, deny-all override by allow maps, group-based triggers, and attribute-based access interactions. Expanded parameterization of existing test to include 'allow' map cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing the 'allow' map to properly process grant cases in the create_claims function, matching the core logic fix in the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

doziedotdev · 2026-03-06T13:27:51Z

Hi @john-westcott-iv sorry about the ping

Could you help review this when you get a chance? You identified the root cause line in AAP-45394. This PR fixes the allow map handling so it correctly grants access instead of only processing the deny case. 6+ support cases linked across AAP-45047 and AAP-45394, both open ~10 months.

john-westcott-iv

📋 Code Review Summary

This PR fixes a bug where map_type='allow' in create_claims() only handled the deny case (has_permission=False). When a trigger fired and granted permission (has_permission=True), the code fell through to the catch-all else branch, logging an erroneous "does not know how to be processed" error and never setting access_allowed = True. This broke the documented deny-all + allow-override pattern (equivalent to AUTH_LDAP_REQUIRE_GROUP).

Files Reviewed: 2 files
Comments Posted: 2 review comments

🔍 Issues Found

0 security/safety issues
0 correctness/logic issues
1 minor suggestion (non-blocking)
1 positive observation

Overall Assessment

LGTM — This is a clean, minimal, and correct fix. The code change is a single-line semantic improvement that makes allow map handling consistent with is_superuser. The tests are comprehensive, covering the main fix scenario, the inverse (deny not overridden without match), group-based matching, and error-log regression. Well done.

General Feedback

The fix correctly implements "last evaluated map wins" semantics for allow maps, which is consistent with how is_superuser maps already work (line 127: is_superuser = has_permission)
Test coverage is thorough: basic parametrized case, error-log regression (AAP-45047), deny-all override (AAP-45394), group-based positive and negative cases
The removed comment ("If any rule does not allow we don't want to return this to true") was documenting the buggy one-way-deny-only behavior — removing it is the right call
6+ support cases across two JIRA tickets — great to see this finally fixed

john-westcott-iv · 2026-03-10T19:21:16Z

-            # If any rule does not allow we don't want to return this to true
-            access_allowed = False
+        if auth_map.map_type == 'allow':
+            access_allowed = has_permission


Clean fix. This now mirrors the is_superuser pattern on line 127 (is_superuser = has_permission), making the handling consistent across map types. The "last evaluated map wins" semantic is correct for ordered map evaluation.

The old code's and not has_permission guard meant allow maps could only deny — they could never grant access back. This simple change restores the intended bidirectional behavior.

john-westcott-iv · 2026-03-10T19:21:16Z

+    authenticator = local_authenticator_map.authenticator
+    res = claims.create_claims(authenticator, "username", {}, [])
+
+    assert res["access_allowed"] is True


Nit (non-blocking): Consider adding a reverse-order test — "allow-all (order=1) then deny-all (order=2)" — to verify that a later deny still correctly overrides an earlier allow. This would complete the ordering matrix:

Map 1 Map 2 Expected

deny-all allow-override True ✅ (this test)

allow-all deny-override False (not tested)

This is minor since the deny path was already working before this fix, but it would document the expected bidirectional "last map wins" contract and guard against future regressions.

github-actions · 2026-04-06T18:25:20Z

DVCS PR Check Results:

PR appears valid (JIRA key(s) found)

Merge branch 'devel' into AAP-45047

30efae9

Merge branch 'devel' into AAP-45047

5822707

john-westcott-iv reviewed Mar 10, 2026

View reviewed changes

doziedotdev added 2 commits April 6, 2026 10:31

Merge branch 'devel' into AAP-45047

b2b0f81

Merge branch 'devel' into AAP-45047

c17d524

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AAP-45047: Fix allow map to process grant case in create_claims#959

AAP-45047: Fix allow map to process grant case in create_claims#959
doziedotdev wants to merge 5 commits intoansible:develfrom
doziedotdev:AAP-45047

doziedotdev commented Mar 2, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 2, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Uh oh!

doziedotdev commented Mar 6, 2026

Uh oh!

john-westcott-iv left a comment

Uh oh!

john-westcott-iv Mar 10, 2026

Uh oh!

john-westcott-iv Mar 10, 2026

Uh oh!

github-actions Bot commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Map 1	Map 2	Expected
deny-all	allow-override	`True` ✅ (this test)
allow-all	deny-override	`False` (not tested)

Conversation

doziedotdev commented Mar 2, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Self-Review Checklist

Testing Instructions

Prerequisites

Steps to Test

Expected Results

Before this fix

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

doziedotdev commented Mar 6, 2026

Uh oh!

john-westcott-iv left a comment

Choose a reason for hiding this comment

📋 Code Review Summary

🔍 Issues Found

Overall Assessment

General Feedback

Uh oh!

john-westcott-iv Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

john-westcott-iv Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

doziedotdev commented Mar 2, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 2, 2026 •

edited

Loading