refactor(BA-3687): consolidate admin_repository into repository for image domain

[MinJuTur]

Summary

• Remove AdminImageRepository class entirely from the image domain
• Move ownership validation from Repository layer to Service layer
• Repository becomes pure data access layer (read/write only)
• Service layer handles SUPERADMIN vs regular user branching with ownership checks
• Add ImageAccessForbiddenError for ownership validation failures
• Add owner_id field to ImageData for service-level validation

Related Issue

• Jira: BA-3687

Architecture Change

Before:

API → Service → Repository (ownership check inside)
         ↘ AdminRepository (no ownership check)

After:

API → Service (ownership check) → Repository (pure data access)

Changed Files

File	Changes
types.py	Add owner_id: Optional[uuid.UUID] field to ImageData
row.py	Add _get_owner_id() helper to extract owner from labels
db_source.py	Remove _validate_image_ownership() and validate_image_ownership(), rename fetch_image_by_id_with_ownership() → fetch_image_by_id()
repository.py	Remove validate_image_ownership(), keep fetch_image_by_id()
service.py	Add _validate_image_ownership() helper for service-level ownership check
repositories.py	Remove admin_repository field
errors/image.py	Add ImageAccessForbiddenError for ownership validation failures
admin_repository.py	Deleted

Design Decisions

Validation in Service Layer

Per reviewer feedback, validation logic should not be in the repository layer. Repository is now pure data access:

# Repository - pure data access
async def fetch_image_by_id(self, image_id, load_aliases) -> ImageData:
    return image_data  # includes owner_id field

# Service - business logic with validation
image_data = await self._repository.fetch_image_by_id(image_id, ...)
self._validate_image_ownership(image_data, user_id)  # raises on failure

owner_id Field in ImageData

Added to support service-level validation. Extracted from image labels (ai.backend.customized-image.owner) in ImageRow._get_owner_id().

Security

• SUPERADMIN: can delete any image (ownership check skipped)
• Regular users: can only delete images they own (ownership validated in service layer)
• Same behavior as before, ownership logic moved from repository to service

Test plan

• All 24 image service unit tests pass
• pants lint passes
• pants check (mypy) passes

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR refactors the image domain architecture by consolidating the AdminImageRepository into the ImageRepository, moving ownership validation logic from the repository layer to the service layer. This creates a cleaner separation where repositories handle pure data access and services handle business logic including authorization.

Key Changes

• Removed the AdminImageRepository class entirely, eliminating code duplication
• Refactored ImageService to perform role-based branching, with SUPERADMIN users bypassing ownership checks and regular users validated via validate_image_ownership
• Simplified repository methods by removing user_id parameters from data modification methods while keeping validate_image_ownership exposed

Reviewed changes

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

Show a summary per file

File	Description
src/ai/backend/manager/repositories/image/admin_repository.py	Entire file deleted - consolidated functionality into ImageRepository
src/ai/backend/manager/repositories/image/repository.py	Simplified methods to pure data access; removed user_id parameters; exposed validate_image_ownership; changed return type of untag_image_from_registry from Optional[ImageData] to ImageData
src/ai/backend/manager/repositories/image/repositories.py	Removed admin_repository field and its instantiation
src/ai/backend/manager/repositories/image/db_source/db_source.py	Removed user_id parameters from mark_user_image_deleted, mark_image_deleted_by_id, and removed combined validation+operation methods
src/ai/backend/manager/services/image/service.py	Added role-based branching logic; SUPERADMIN users skip validation; regular users call validate_image_ownership before operations; added ImageRef import for type annotations
src/ai/backend/manager/services/processors.py	Updated ImageService instantiation to remove admin_repository parameter
tests/unit/manager/services/image/conftest.py	Removed mock_admin_image_repository fixture and its usage in image_service fixture
tests/unit/manager/services/image/test_image_service.py	Updated all SUPERADMIN tests to use mock_image_repository instead of mock_admin_image_repository; updated forbidden access tests to mock validate_image_ownership and resolve_image calls

---

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

[Copilot]

This implementation makes three database queries for non-SUPERADMIN users: one in resolve_image to get the image data, one in validate_image_ownership to validate ownership, and a third in soft_delete_image which internally calls _resolve_image again. Consider optimizing to reduce to two queries by removing the explicit resolve_image call since validate_image_ownership already fetches the image and validates ownership, or by having soft_delete_image accept an optional pre-resolved image.

Suggested change

	data = await self._image_repository.soft_delete_image(identifiers)
	data = await self._image_repository.soft_delete_image_by_id(image_data.id)
	else:
	data = await self._image_repository.soft_delete_image(identifiers)

[Copilot]

The result of validate_image_ownership (which returns ImageData) is being discarded, then the image is fetched again by delete_image_with_aliases. This makes two database queries when only one is needed. Consider using the ImageData returned from validate_image_ownership if the delete operation doesn't need to refetch the latest state, or have the delete method accept pre-fetched image data to avoid the redundant query.

[Copilot]

The result of validate_image_ownership (which returns ImageData) is being discarded, then the image is fetched again by untag_image_from_registry. This makes two database queries when only one is needed. Consider using the ImageData returned from validate_image_ownership if the untag operation doesn't need to refetch the latest state, or have the untag method accept pre-fetched image data to avoid the redundant query.

[jopemachine]

I think we need to add some service-level test cases to verify behavior changes based on client_role.

[jopemachine]

Could we also add test cases that cover exception scenarios, not just the success cases?
Adding test cases that succeed or fail depending on the client_role would help verify that permission checks in the service logic are working correctly.

[MinJuTur]

Could we also add test cases that cover exception scenarios, not just the success cases? Adding test cases that succeed or fail depending on the client_role would help verify that permission checks in the service logic are working correctly.

I'd like to clarify what additional test cases you're looking for. Currently, we have the following test cases for each method (forget_image, forget_image_by_id, purge_image_by_id, untag_image_from_registry):

Could you clarify which specific exception scenarios or client_role-based test cases are missing?

[jopemachine]

It doesn’t make sense to raise a ForgetImageForbiddenError when the user lacks the required permissions.

[jopemachine]

The conventions used in tests/unit/manager/services/image/test_image_service.py are outdated.
We should remove conftest.py and move the existing fixtures into the TestImageService test class in test_image_service.py.
(Please refer to the tests of other services as a reference like TestAuditLogService.)

However, this would significantly increase the scope of this PR.
Could you create a follow-up PR to handle this refactoring after this PR is merged? @MinJuTur