test(BA-3589): add tests for manager's auth api handler #7653

kwonkwonn · 2025-12-31T02:38:32Z

Increase test coverage before migrating api/auth.py to follow pydantic model.
This Pr specifically contains 'validate_ip' and 'validate decorator' which will be used before and after migration of auth.py

Checklist: (if applicable)

Milestone metadata specifying the target backport version
[v] Mention to the original issue
Installer updates including:
- Fixtures for db schema changes
- New mandatory config options
Update of end-to-end CLI integration tests in ai.backend.test
API server-client counterparts (e.g., manager API -> client SDK)
[v] Test case(s) to:
- Demonstrate the difference of before/after
- Demonstrate the flow of abstract/conceptual models with a concrete implementation
Documentation
- Contents in the docs directory
- docstrings in public interfaces and type annotations

cla-assistant · 2025-12-31T02:38:48Z

All committers have signed the CLA.

Copilot

Pull request overview

This PR adds comprehensive unit tests for the validate_ip function and authentication decorators (auth_required, admin_required, superadmin_required) in preparation for migrating api/auth.py to follow the pydantic model. The tests establish a baseline to ensure safe refactoring with regression detection.

Key Changes

Added test coverage for IP validation logic (allow/deny scenarios)
Added test coverage for authentication decorator behavior and authorization checks
Added tests to verify decorator attributes are properly set

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-31T02:43:16Z

tests/component/manager/api/test_auth.py

+def test_validate_ip_allowed() -> None:
+    """IP within CIDR range should be allowed, empty allowlist allows all."""
+    request = MagicMock(spec=web.Request)
+    request.headers = {"X-Forwarded-For": "10.0.0.50"}
+    request.remote = None
+
+    # Empty allowlist allows all IPs
+    user: dict[str, Any] = {"allowed_client_ip": None}
+    validate_ip(request, user)
+
+    # IP within CIDR range is allowed
+    user = {"allowed_client_ip": [ReadableCIDR("10.0.0.0/24", is_network=True)]}
+    validate_ip(request, user)
+
+
+def test_validate_ip_denied() -> None:
+    """IP not in allowlist should raise AuthorizationFailed."""
+    request = MagicMock(spec=web.Request)
+    request.headers = {"X-Forwarded-For": "192.168.1.100"}
+    request.remote = None
+
+    user: dict[str, Any] = {"allowed_client_ip": [ReadableCIDR("10.0.0.0/24", is_network=True)]}
+    with pytest.raises(AuthorizationFailed, match="not allowed IP address"):
+        validate_ip(request, user)


The test coverage for validate_ip is incomplete. Several edge cases from the implementation are not tested:

Invalid IP address format - should raise InvalidAuthParameters

Missing client IP (both X-Forwarded-For and request.remote are None) - should raise AuthorizationFailed with "Not allowed IP address"

Non-list allowed_client_ip value - should raise InvalidClientIPConfig

Empty list allowed_client_ip behavior (currently only None is tested)

These test cases should be added to ensure comprehensive coverage before the migration mentioned in the PR description.

Copilot · 2025-12-31T02:43:16Z

tests/component/manager/api/test_auth.py

+from ai.backend.manager.errors.auth import (
+    AuthorizationFailed,
+    InvalidAuthParameters,
+)


The InvalidClientIPConfig exception is not imported but should be included to test the edge case where allowed_client_ip is not a list (line 397-398 in auth.py). Add this import to the error imports section to enable comprehensive test coverage.

Copilot · 2025-12-31T02:43:17Z

tests/component/manager/api/test_auth.py

+    @pytest.mark.asyncio
+    async def test_superadmin_required_sets_handler_attr(self) -> None:
+        """superadmin_required should set handler attributes."""
+
+        @superadmin_required
+        async def handler(request: web.Request) -> web.Response:
+            return web.Response(text="ok")
+
+        # get_handler_attr expects request.match_info.handler, so we check _backend_attrs directly
+        attrs = getattr(handler, "_backend_attrs", {})
+        assert attrs.get("auth_required", False) is True
+        assert attrs.get("auth_scope", None) == "superadmin"


Missing test case for superadmin_required when the request is not authorized (is_authorized=False). Currently, only the cases where is_authorized=True with is_superadmin=True and is_superadmin=False are tested. Add a test case for when is_authorized=False to ensure complete coverage of the decorator's authorization checks, similar to test_admin_required_unauthorized.

HyeockJinKim

It's easier to check the implementation details while writing test code, rather than focusing on those details.
Please write test code prioritizing what behavior should be satisfied.

Additionally, please include the JIRA issue number in the PR title under "test:" sections. (e.g. test(BA-...): ...)

tests/component/manager/api/test_auth.py

HyeockJinKim · 2025-12-31T04:23:10Z

And the timeline-check part in CI got stuck, please check how it applies to other PRs and changes/directories. (Refer to https://github.com/lablup/backend.ai?tab=contributing-ov-file#4-write-changelog-fragment-towncrier.)
You can see which items are available by checking the pyproject.toml file.

HyeockJinKim

I'm concerned that there are too many utility functions in this work. Ideally, we should keep test-related objects with simpler mocking objects, rather than multiple implementations. (Considering using aiohttp's Fixture as well.)

kwonkwonn · 2026-01-06T04:22:16Z

Thanks for the review!

I thought the same thing while writing the code, but at the same time, I thought it will be easier to test other manager/api handlers which most of them follow the root_ctx: RootContext = request.app["_root.context"] pattern with this class.

If I minimize utility functions but keep APITestAppBuilder, would that be acceptable?

kwonkwonn · 2026-01-06T04:31:30Z

Or I can fix it to look something more like tests/unit/manager/api/storage_namespace/ which will make each test file simpler.

kwonkwonn · 2026-01-06T05:32:06Z

This commit implements minimalized mocking utility for test auth.py, referenced vfolder test scenarios, i think we can move create_auth_app outside to esier referencing from other http testing.

HyeockJinKim

I think it would be better to create a fixture that registers each handler to the application and sends a request to verify it. Please take a look at https://docs.aiohttp.org/en/stable/testing.html#unittest for reference.
Could I create a sample test and share it? @kwonkwonn

fregataa

Did we decide to implement fixture in a separate conftest file? @HyeockJinKim

tests/unit/manager/api/auth/conftest.py

tests/unit/manager/api/auth/test_handlers.py

kwonkwonn · 2026-01-06T07:21:12Z

Could I create a sample test and share it? @kwonkwonn

yes, If it's not bothering you so much, i will be very appreciated!
@HyeockJinKim

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/unit/manager/api/auth/test_handlers.py

Copilot · 2026-01-07T09:15:57Z

tests/unit/manager/api/auth/test_handlers.py

+    @pytest.mark.asyncio
+    async def test_returns_bad_request_on_failure(
+        self,
+        authorized_request: MagicMock,
+        mock_root_ctx: MagicMock,
+    ) -> None:
+        """Verify BAD_REQUEST is returned when password update fails."""
+        user_uuid = uuid.uuid4()
+        authorized_request.text = AsyncMock(
+            return_value=json.dumps({
+                "old_password": "oldpass",
+                "new_password": "newpass",
+                "new_password2": "differentpass",
+            })
+        )
+        authorized_request["user"] = {
+            "uuid": user_uuid,
+            "email": "[email protected]",
+            "domain_name": "default",
+        }
+        authorized_request["keypair"] = {"access_key": "AKTEST"}
+        mock_root_ctx.processors.auth.update_password.wait_for_complete = AsyncMock(
+            return_value=UpdatePasswordActionResult(success=False, message="mismatch")
+        )
+
+        response = await update_password(authorized_request)
+
+        assert response.status == HTTPStatus.BAD_REQUEST


The test verifies that BAD_REQUEST status is returned on password update failure, but doesn't check the error message in the response body. The handler returns a specific error message 'new password mismatch' that should be verified to ensure the correct error information is provided to users.

fregataa

Looks good. Please check the comment and check the hard coded values

fregataa · 2026-01-07T09:19:17Z

tests/unit/manager/api/auth/test_handlers.py

+        """Verify processor is called with correct params."""
+        user_uuid = uuid.uuid4()
+        authorized_request.text = AsyncMock(
+            return_value=json.dumps({"email": "[email protected]", "full_name": "New Name"})


How about using a variable for "full_name" rather than using a hard coded value? Because this should be tested by comparing

modified "full_name" to be compared as variable.
thank you!

What I mean is that we need to check all values that are compared for testing and change them to variables, not just the full_name case.

For example:

# OK name = "hello" result = func(name) assert result.name == name # NOT result = func("hello") assert result.name == "hello"

Oh i misunderstand that.
I'll fix it to be in variable form

For any value which is subject of comparions, I move it out from function scope and move it aside to class fields.
which can increase flexability for future test scenarios.

thank you for concise review!

fregataa

LGTM

fregataa · 2026-01-09T08:37:40Z

Please rebase to main

Copilot AI review requested due to automatic review settings December 31, 2025 02:38

github-actions bot assigned kwonkwonn Dec 31, 2025

github-actions bot added the size:L 100~500 LoC label Dec 31, 2025

kwonkwonn marked this pull request as draft December 31, 2025 02:38

Copilot started reviewing on behalf of kwonkwonn December 31, 2025 02:38 View session

Copilot AI reviewed Dec 31, 2025

View reviewed changes

HyeockJinKim requested changes Dec 31, 2025

View reviewed changes

tests/component/manager/api/test_auth.py Outdated Show resolved Hide resolved

tests/component/manager/api/test_auth.py Outdated Show resolved Hide resolved

kwonkwonn changed the title ~~test: add tests for validate_ip and auth decorators~~ test(BA-3589) add tests for validate_ip and auth decorators Dec 31, 2025

kwonkwonn changed the title ~~test(BA-3589) add tests for validate_ip and auth decorators~~ test(BA-3589): add tests for validate_ip and auth decorators Dec 31, 2025

HyeockJinKim force-pushed the main branch 2 times, most recently from 9552aac to 4af738e Compare December 31, 2025 15:41

kwonkwonn force-pushed the test/auth-api-coverage branch from ff73214 to 1ef5bcb Compare January 6, 2026 04:00

github-actions bot added size:XL 500~ LoC Intern-OKR and removed size:L 100~500 LoC labels Jan 6, 2026

HyeockJinKim requested changes Jan 6, 2026

View reviewed changes

kwonkwonn force-pushed the test/auth-api-coverage branch from 1ef5bcb to 30ccaff Compare January 6, 2026 05:31

kwonkwonn changed the title ~~test(BA-3589): add tests for validate_ip and auth decorators~~ test(BA-3589): add tests for manager's auth api handler Jan 6, 2026

HyeockJinKim reviewed Jan 6, 2026

View reviewed changes

fregataa reviewed Jan 6, 2026

View reviewed changes

tests/unit/manager/api/auth/conftest.py Outdated Show resolved Hide resolved

tests/unit/manager/api/auth/test_handlers.py Outdated Show resolved Hide resolved

tests/unit/manager/api/auth/test_handlers.py Outdated Show resolved Hide resolved

kwonkwonn force-pushed the test/auth-api-coverage branch from 30ccaff to 4ab96da Compare January 7, 2026 08:21

kwonkwonn marked this pull request as ready for review January 7, 2026 09:10

kwonkwonn requested review from HyeockJinKim and Copilot January 7, 2026 09:11

kwonkwonn requested a review from fregataa January 7, 2026 09:11

Copilot started reviewing on behalf of kwonkwonn January 7, 2026 09:11 View session

Copilot AI reviewed Jan 7, 2026

View reviewed changes

fregataa reviewed Jan 7, 2026

View reviewed changes

kwonkwonn force-pushed the test/auth-api-coverage branch 3 times, most recently from a82a84d to 0f73da8 Compare January 7, 2026 11:21

kwonkwonn requested a review from fregataa January 7, 2026 11:28

kwonkwonn mentioned this pull request Jan 8, 2026

refactor(BA-3590): Implement Pydantic migration for manager/api/auth.py #7865

Open

7 tasks

fregataa approved these changes Jan 8, 2026

View reviewed changes

kwonkwonn force-pushed the test/auth-api-coverage branch from 0f73da8 to 5324bc3 Compare January 9, 2026 01:21

test: add tests for auth route handlers type validation

a5081cf

kwonkwonn force-pushed the test/auth-api-coverage branch from 5324bc3 to a5081cf Compare January 9, 2026 08:51

HyeockJinKim approved these changes Jan 9, 2026

View reviewed changes

HyeockJinKim added this pull request to the merge queue Jan 9, 2026

Merged via the queue into main with commit 9050736 Jan 9, 2026
28 checks passed

HyeockJinKim deleted the test/auth-api-coverage branch January 9, 2026 18:46

test(BA-3589): add tests for manager's auth api handler #7653

test(BA-3589): add tests for manager's auth api handler #7653

Uh oh!

Conversation

kwonkwonn commented Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cla-assistant bot commented Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Uh oh!

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

Uh oh!

HyeockJinKim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

HyeockJinKim commented Dec 31, 2025

Uh oh!

HyeockJinKim left a comment

Choose a reason for hiding this comment

Uh oh!

kwonkwonn commented Jan 6, 2026

Uh oh!

kwonkwonn commented Jan 6, 2026

Uh oh!

kwonkwonn commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyeockJinKim left a comment

Choose a reason for hiding this comment

Uh oh!

fregataa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwonkwonn commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

fregataa left a comment

Choose a reason for hiding this comment

Uh oh!

fregataa Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

kwonkwonn Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregataa Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwonkwonn Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

kwonkwonn Jan 7, 2026

kwonkwonn commented Dec 31, 2025 •

edited

Loading

cla-assistant bot commented Dec 31, 2025 •

edited

Loading

kwonkwonn commented Jan 6, 2026 •

edited

Loading

kwonkwonn commented Jan 6, 2026 •

edited

Loading

kwonkwonn Jan 7, 2026 •

edited

Loading

fregataa Jan 7, 2026 •

edited

Loading