Skip to content

More intuitive Authorization checks#743

Merged
byewokko merged 3 commits intomasterfrom
feature/intuitive-auth-checks
Apr 2, 2026
Merged

More intuitive Authorization checks#743
byewokko merged 3 commits intomasterfrom
feature/intuitive-auth-checks

Conversation

@byewokko
Copy link
Copy Markdown
Collaborator

@byewokko byewokko commented Apr 2, 2026

Issue

Authorization's methods has_resource_access, has_tenant_access and has_superuser_access raise NotAuthenticatedError when the authorization is expired. This is a rather surprising behavior; a return value of False would be more useful.

Solution

Authorization.has_*_access now return False when the authorization is expired. This is a potentially breaking change.

Summary by CodeRabbit

  • Bug Fixes

    • Permission checks now return false for expired/invalid authorizations instead of triggering errors, allowing smoother permission handling.
    • Methods that enforce access still raise when required, preserving strict checks where callers explicitly demand validity.
  • Changes

    • Expired authorizations are presented more clearly in diagnostics.

@byewokko byewokko self-assigned this Apr 2, 2026
@byewokko byewokko added the enhancement New feature or request label Apr 2, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 2, 2026

📝 Walkthrough

Walkthrough

Changed Authorization behavior so has_* checks return False when the authorization is expired/invalid instead of raising; require_* methods still call require_valid() and raise NotAuthenticatedError when appropriate. __repr__ now checks validity before building superuser-aware output.

Changes

Cohort / File(s) Summary
Authorization Implementation
asab/web/auth/authorization.py
Adjusted __repr__ to use self.is_valid() before constructing representation; changed has_superuser_access, has_resource_access, and has_tenant_access to gate checks with self.is_valid() and return False when expired; updated require_superuser_access and require_resource_access to call require_valid() explicitly; updated docstrings to remove NotAuthenticatedError from has_* methods.
Authorization Tests
test/test_auth/test_authorization.py
Updated TestExpiredSuperuser to assert has_resource_access(...), has_tenant_access(...), and has_superuser_access() return False for expired auth (using assertFalse) while preserving assertRaises(NotAuthenticatedError) for the corresponding require_* methods.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly Related PRs

  • #742 — Modifies Authorization.__repr__() in the same file to use is_valid() and avoid raising NotAuthenticatedError.

Poem

🐰 I nibble at expired keys with care,
Booleans whisper, "No" instead of scare.
Guards still shout when rules are breached,
While gentle checks keep flows unclenched—
A hop, a fix, a tidy patch declared. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title directly and clearly describes the main change: making Authorization checks more intuitive by changing how expired authorizations are handled (returning False instead of raising exceptions).
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/intuitive-auth-checks

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@asab/web/auth/authorization.py`:
- Around line 182-183: The require_* methods call require_valid() and then
re-run has_*(), which can turn an intended NotAuthenticatedError into
AccessDeniedError if expiry passes between calls; change each require_* (e.g.,
require_superuser_access, require_resources) to call require_valid() first and
then invoke the underlying predicate directly instead of has_*() so no second
validity check occurs (for example call the internal predicate used by
has_superuser_access and _resources — e.g., self._has_superuser_access(...) /
self._has_resources(...) — rather than self.has_superuser_access()), and apply
the same change to the other occurrences that follow the same pattern.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: b84c559e-a5ec-4178-945a-d72bbcb13e78

📥 Commits

Reviewing files that changed from the base of the PR and between bed858f and 9f04cc7.

📒 Files selected for processing (2)
  • asab/web/auth/authorization.py
  • test/test_auth/test_authorization.py

@byewokko byewokko added the breaking change This will introduce a breaking change label Apr 2, 2026
@byewokko byewokko merged commit 4e1e467 into master Apr 2, 2026
14 checks passed
@byewokko byewokko deleted the feature/intuitive-auth-checks branch April 2, 2026 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change This will introduce a breaking change enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant