Skip to content

feat: add runtime system settings module#19

Merged
kemalcalak merged 12 commits into
mainfrom
system-settings
Jun 16, 2026
Merged

feat: add runtime system settings module#19
kemalcalak merged 12 commits into
mainfrom
system-settings

Conversation

@kemalcalak

Copy link
Copy Markdown
Owner

No description provided.

@kemalcalak kemalcalak self-assigned this Jun 16, 2026
@kemalcalak

Copy link
Copy Markdown
Owner Author

/review — 7 bulgu

1. resolve_optional_user is_active kontrolünü atlıyor

Dosya: app/api/deps.py#L198-L226
Kategori: bug · Önem: major

Fonksiyonun docstring'i get_ws_user'ın HTTP karşılığı olduğunu söylüyor; ancak get_ws_user kullanıcıyı döndürmeden önce if user is None or not user.is_active: return None kontrolü yaparken resolve_optional_user is_active'e bakmadan döndürüyor. Deaktive edilmiş ama token'ı iptal edilmemiş bir admin, bakım modu kapısını aşabilir. (İki ayrı reviewer bağımsız olarak tespit etti.)

get_ws_user: if user is None or not user.is_active: return None — resolve_optional_user'da bu kontrol yok.

Öneri: Son satırı user = await get_user_by_id(...); return user if (user and user.is_active) else None olarak değiştir.


2. BRANDING_LOGO kategorisi yetkisiz kullanıcıya açık

Dosya: app/services/file_service.py#L73-L78
Kategori: bug · Önem: major

FileCategory.BRANDING_LOGO için site-geneli bir Cloudinary klasör yolu yazıldı, ancak yalnızca admin/superadmin'in bu kategoriyle yükleme yapabileceğini doğrulayan bir RBAC kontrolü yok. /upload endpoint'i CurrentActiveUser ile korunduğundan herhangi bir aktif kullanıcı category=branding_logo ile site branding klasörüne dosya yazabilir.

if category is FileCategory.BRANDING_LOGO:  folder = f"{settings.CLOUDINARY_UPLOAD_FOLDER}/{category.value}"

Öneri: Kategori kontrolünün öncesine if category is FileCategory.BRANDING_LOGO and current_user.role not in (SystemRole.ADMIN, SystemRole.SUPERADMIN): raise HTTPException(403, ErrorMessages.INSUFFICIENT_PERMISSIONS) guard'ı ekle.


3. core/ katmanı api/ katmanına bağımlı (katman inversiyonu)

Dosya: app/core/middleware.py#L14
Kategori: review-md-compliance · Önem: major

maintenance_mode_middleware, resolve_optional_user ve get_db'yi app.api.deps'ten import ediyor. core/ tüm katmanların import ettiği taban katmandır; api/'dan import yapması bağımlılık yönünü tersine çevirir.

REVIEW.md §3.1: "api → services → repositories → models — bu sıra bozulamaz."

Öneri: resolve_optional_userapp/core/security.py veya app/utils/'e, get_db'yi app/core/db.py'ye taşı; böylece core/ → api/ bağımlılığı ortadan kalkar.


4. maintenance_mode_middleware için test yok

Dosya: app/core/middleware.py#L54-L101
Kategori: review-md-compliance · Önem: major

Bakım modu middleware'i tüm API trafiğini kapı altına alır ve hatalı çalışması tüm kullanıcıları etkiler, ancak davranışını doğrulayan test yok. Admin bypass, exempt path mantığı ve session açma akışı test kapsamı dışında.

REVIEW.md §5.9: "Yeni endpoint için test yok (auth/security/payment-impacting'se major)."

Öneri: Bakım açıkken non-admin → 503, exempt path'ler (login, /settings/public, health) açık, admin geçebiliyor senaryolarını test et.


5. PATCH /admin/system-settings/{key} için test yok

Dosya: app/api/routes/admin/system_settings.py#L1-L60
Kategori: review-md-compliance · Önem: major

PATCH /admin/system-settings/{key} güvenlik etkili ayarları (maintenance_mode, registration_enabled) değiştirir, ancak RBAC, tip doğrulama, bilinmeyen anahtar ve başarılı güncelleme senaryolarını kapsayan test yok.

REVIEW.md §5.9 / §6.7: auth/güvenlik etkili mutasyon endpoint'leri için test major önceliklidir.

Öneri: app/tests/test_system_settings.py ekle: 404 (bilinmeyen key), 422 (yanlış tip), 403 (yetersiz izin), 200 (başarılı güncelleme).


6. Servis fonksiyonları _service suffix'ini taşımıyor

Dosya: app/services/system_settings.py#L66-L83
Kategori: review-md-compliance · Önem: minor

list_all, get_public_settings ve update_setting, kod tabanındaki diğer servislerin (update_user_service, deactivate_own_account_service) tutarlı kullandığı adlandırma kuralına uymuyor.

REVIEW.md §3.16: "Service: <verb>_<resource>_service."

Öneri: list_all_settings_service, get_public_settings_service, update_setting_service olarak yeniden adlandır ve route importlarını güncelle.


7. Repository fonksiyonları entity adı içermiyor

Dosya: app/repositories/system_setting.py#L11-L52
Kategori: review-md-compliance · Önem: minor

get_by_key, get_all ve upsert, mevcut repolardaki (get_user_by_id, create_user) <verb>_<entity> kalıbıyla uyumsuz.

REVIEW.md §3.16: "Repository: <verb>_<entity> (get_user_by_id, create_user, deactivate_user)."

Öneri: get_system_setting_by_key, get_all_system_settings, upsert_system_setting olarak yeniden adlandır ve services/system_settings.py importlarını güncelle.


🤖 Generated with Claude Code

@kemalcalak

Copy link
Copy Markdown
Owner Author

/review — bulgular giderildi ✅

Önceki review'daki 7 bulgunun tamamı çözüldü:

# Bulgu Çözüm
1 resolve_optional_user is_active atlıyor deps.py — deaktive kullanıcı artık None döner (get_ws_user ile aynı davranış)
2 BRANDING_LOGO yetkisiz erişime açık file_service.py — admin/superadmin RBAC guard'ı eklendi
3 core/ → api/ katman inversiyonu Maintenance middleware app/api/middleware.py'ye taşındı; core/ artık api/'dan import etmiyor
4 Maintenance middleware testi yok tests/admin/test_system_settings.py — 503/admin-bypass/exempt path testleri
5/8 Admin & public endpoint testleri yok Aynı dosya — list, patch, 404, 422, 403 senaryoları
6 Servis fonksiyonları _service suffix taşımıyor list_all_settings_service, get_public_settings_service, update_setting_service
7 Repository fonksiyonları entity adı içermiyor get_system_setting_by_key, get_all_system_settings, upsert_system_setting

Ek olarak BRANDING_LOGO için pozitif/negatif test eklendi (regular user → 403, admin → 201 + site-geneli klasör).

Doğrulama:

  • tests/admin/test_system_settings.py: 12 passed
  • tests/test_files.py: 12 passed
  • ruff check: temiz

🤖 Generated with Claude Code

@kemalcalak kemalcalak merged commit df8c5f1 into main Jun 16, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant