feature/auth-hardening #24

Open
wikiker wants to merge 3 commits from feature/auth-hardening into dev
Owner

Fixes #16

Fixes #16
Security fixes:
- Added @login_required to record_lookup to prevent enumeration attacks
- Added admin_required decorator for staff-only views
- Added user_permissions_overview view (staff only)
- Added user_permissions.html template
- Added 'Uživatelé' link in navbar for staff

Documentation:
- Added permissions section to README.md
- Documented roles, how to assign permissions, security measures

Tests:
- Added test_auth_security.py with 11 tests
- Tests cover: record_lookup security, overview access, user permissions
- All 46 tests pass (1 skipped due to FIO_API_TOKEN config)

Addresses issues from AUTH.md:
- Issue 2: record_lookup now requires authentication
- Issue 1: Django admin login forms still need attention
wikiker self-assigned this 2026-10-24 13:03:49 +02:00
setnicka left a comment

Doklady přes /media/ jsou pořád dostupné bez přihlášení (issue #15). Chtělo by to přidat routu, která ověřuje práva, a defaultní routu /media/ smazat.

Doklady přes `/media/` jsou pořád dostupné bez přihlášení (issue #15). Chtělo by to přidat routu, která ověřuje práva, a defaultní routu `/media/` smazat.
@ -51,6 +51,25 @@ Ve webovém rozhraní je možné požádat o výpis z bankovního účtu u Fio b
**Důležité**: Pokud se požadují data starší než 90 dní, je nutné manuálně autorizovat API klíč v internetovém bankovnictví Fio banky.
## Oprávění
Collaborator

Drobnost: překlep

Drobnost: překlep
@ -54,0 +63,4 @@
| **Přihlášený** | Základní | Může prohlížet semináře, ke kterým má přístup |
| **Účetní (accountant)** | Rozšířený | Může spravovat transakce, proplácení, importovat výpisy z banky |
| **Správce (staff)** | Administrativní | Může spravovat uživatele a jejich oprávnění |
| **Superuživatel** | Plný | Má všechny práva včetně správy Django adminu |
Collaborator

Pár drobností k tomu, co je tady napsáno:

  • Nepřihlášený: registraci teď neuvidí nikdo, protože ještě není, ne?
  • Přihlášený může aktuálně upravovat data u seminářů, ke kterým má přístup.
  • Staff: V aktuálním systému práv (který je trochu překomplikovaný, viz samostatná zpráva) na to samotné is_staff nestačí, je na to potřeba ještě auth.change_user nebo být superuživatelem

Takhle je to teď více odpovídající realitě:

Role Co může
Nepřihlášený jen přihlášení
Organizátor semináře (Seminar.organizers) prohlížet, upravovat a mazat záznamy svých seminářů, zakládat akce
Účetní (budget.accountant) vše v aplikaci: všechny semináře, přehled, přidávání, import z banky, proplácení, hromadná úprava
Staff (is_staff) vstup do Django adminu a přehled uživatelů. V adminu jen modely, ke kterým má práva
Superuživatel vše, včetně správy uživatelů v adminu
Pár drobností k tomu, co je tady napsáno: * Nepřihlášený: registraci teď neuvidí nikdo, protože ještě není, ne? * Přihlášený může aktuálně **upravovat** data u seminářů, ke kterým má přístup. * Staff: V aktuálním systému práv (který je trochu překomplikovaný, viz samostatná zpráva) na to samotné `is_staff` nestačí, je na to potřeba ještě `auth.change_user` nebo být superuživatelem Takhle je to teď více odpovídající realitě: | Role | Co může | |---------|---------| | Nepřihlášený |jen přihlášení | | Organizátor semináře (`Seminar.organizers`) | prohlížet, upravovat a mazat záznamy svých seminářů, zakládat akce | | Účetní (`budget.accountant`) | vše v aplikaci: všechny semináře, přehled, přidávání, import z banky, proplácení, hromadná úprava | | Staff (`is_staff`) | vstup do Django adminu a přehled uživatelů. V adminu jen modely, ke kterým má práva | | Superuživatel | vše, včetně správy uživatelů v adminu |
@ -79,0 +79,4 @@
def admin_required(view_func):
"""Restricts view to staff users only."""
Collaborator

Nebyl by (vzhledem ke komentáři a kódu) lepší název staff_required?

Nebyl by (vzhledem ke komentáři a kódu) lepší název `staff_required`?
@ -0,0 +58,4 @@
<ul class="unstyled-list text-small">
{% for perm in user_info.permissions %}
<li>{{ perm }}</li>
{% endfor %}
Collaborator

Superuživatel tady nebude mít nic vypsaného, i když má všechna práva. Možná speciálně v permissions vypsat že je to superuživatel?

Superuživatel tady nebude mít nic vypsaného, i když má všechna práva. Možná speciálně v permissions vypsat že je to superuživatel?
@ -0,0 +48,4 @@
def test_record_lookup_enumeration_blocked(self):
"""Test that records cannot be enumerated without authentication."""
# This simulates the attack described in AUTH.md
Collaborator

AUTH.md? Žádné nevidím. Na necommitnuté soubory by se nemělo nic odkazovat.

AUTH.md? Žádné nevidím. Na necommitnuté soubory by se nemělo nic odkazovat.
@ -0,0 +60,4 @@
self.assertEqual(response.status_code, 302)
# No records should be found
self.assertEqual(len(found_records), 0)
Collaborator

Tenhle assert projde vždycky, do found_records se nic nikde nepřidává.

Tenhle assert projde vždycky, do `found_records` se nic nikde nepřidává.
@ -320,3 +321,4 @@
@login_required
def record_lookup(request):
Collaborator

@login_required je málo, teď to pořád vrátí všechny záznamy z daného dne všem přihlášeným, i když v README se píše:

| Přihlášený | Základní | Může prohlížet semináře, ke kterým má přístup |

Buď to chce předefinovat co kdo může (v README), nebo tady přidat lepší kontrolu / filtrování, nebo to omezit jenom na accountant.

`@login_required` je málo, teď to pořád vrátí všechny záznamy z daného dne všem přihlášeným, i když v README se píše: > | **Přihlášený** | Základní | Může prohlížet semináře, ke kterým má přístup | Buď to chce předefinovat co kdo může (v README), nebo tady přidat lepší kontrolu / filtrování, nebo to omezit jenom na accountant.
@ -326,3 +328,4 @@
Requires authentication to prevent enumeration attacks.
"""
code = request.GET.get("code", "").strip()
Collaborator

(Forgejo neumí dát komentář níž, tak ho dávám sem)

record_lookup pořád spadne na nevalidním vstupu. Za regex bych přidal něco jako:

if not match:
    return JsonResponse({"error": "invalid code"}, status=400)
try:
    date = datetime(
        int(match.group("year")), int(match.group("month")), int(match.group("day"))
    ).date()
except ValueError:
    return JsonResponse({"error": "invalid date"}, status=400)
(Forgejo neumí dát komentář níž, tak ho dávám sem) `record_lookup` pořád spadne na nevalidním vstupu. Za regex bych přidal něco jako: ```python if not match: return JsonResponse({"error": "invalid code"}, status=400) try: date = datetime( int(match.group("year")), int(match.group("month")), int(match.group("day")) ).date() except ValueError: return JsonResponse({"error": "invalid date"}, status=400) ```
@ -449,0 +464,4 @@
perms = set()
for group in user.groups.all():
perms.update(group.permissions.values_list("codename", flat=True))
perms.update(user.user_permissions.values_list("codename", flat=True))
Collaborator

Pozor, prefetch_related se tu nevyužije: values_list() vytvoří nový queryset a ten pošle vlastní dotaz, takže se na každého uživatele (a každou jeho skupinu) pošle dotaz navíc. Přednačtená data čte jen .all().

Stačí přednačíst i práva skupin a pole vytáhnout v Pythonu:

users = User.objects.prefetch_related("groups__permissions", "user_permissions").order_by("username")

a

for group in user.groups.all():
    perms.update(p.codename for p in group.permissions.all())
perms.update(p.codename for p in user.user_permissions.all())
Pozor, `prefetch_related` se tu nevyužije: `values_list()` vytvoří nový queryset a ten pošle vlastní dotaz, takže se na každého uživatele (a každou jeho skupinu) pošle dotaz navíc. Přednačtená data čte jen `.all()`. Stačí přednačíst i práva skupin a pole vytáhnout v Pythonu: ```python users = User.objects.prefetch_related("groups__permissions", "user_permissions").order_by("username") ``` a ```python for group in user.groups.all(): perms.update(p.codename for p in group.permissions.all()) perms.update(p.codename for p in user.user_permissions.all()) ```
@ -449,0 +471,4 @@
"is_staff": user.is_staff,
"is_superuser": user.is_superuser,
"permissions": sorted(perms),
})
Collaborator

V tomhle výpisu úplně schází semináře, jejich je uživatel členem.

Dál tu schází třeba stav, jestli je aktivní nebo ne (když už máme to políčko is_active v databázi).

V tomhle výpisu úplně schází semináře, jejich je uživatel členem. Dál tu schází třeba stav, jestli je aktivní nebo ne (když už máme to políčko `is_active` v databázi).
Collaborator

A ještě: systém práv je teď v aplikaci hrozně překomplikovaný, protože Django si tam dogenerovává tunu vlastních práv, která ale nikde nepoužíváme. Jestli dobře počítám, tak jich je přes 50 různých (třeba budget.{add,change,delete,view}_{seminar,event,record,banktransaction,virtualtransaction} + podobně rozsáhlé sety na invoicer. a auth.).

Bylo by pěkné systém práv nějak rozumně popsat, zbavit se tohohle hrozného balastu automatických práv a používat jenom naše práva. Přemýšlím, jestli pak nedává smysl mít práva scoped = můžeš mít právo accountant(*) nebo třeba jenom accountant(KSP) a mít to všechno navázané na tohle 🤷 Ale to by chtělo asi nejdřív zkusit někam sepsat.

Současná práva viz dotaz do SQLite:

sqlite> SELECT * FROM auth_permission;
id  content_type_id  codename                   name                                          
--  ---------------  -------------------------  ----------------------------------------------
1   1                add_logentry               Can add log entry                             
2   1                change_logentry            Can change log entry                          
3   1                delete_logentry            Can delete log entry                          
4   1                view_logentry              Can view log entry                            
5   2                add_permission             Can add permission                            
6   2                change_permission          Can change permission                         
7   2                delete_permission          Can delete permission                         
8   2                view_permission            Can view permission                           
9   3                add_group                  Can add group                                 
10  3                change_group               Can change group                              
11  3                delete_group               Can delete group                              
12  3                view_group                 Can view group                                
13  4                add_user                   Can add user                                  
14  4                change_user                Can change user                               
15  4                delete_user                Can delete user                               
16  4                view_user                  Can view user                                 
17  5                add_contenttype            Can add content type                          
18  5                change_contenttype         Can change content type                       
19  5                delete_contenttype         Can delete content type                       
20  5                view_contenttype           Can view content type                         
21  6                add_session                Can add session                               
22  6                change_session             Can change session                            
23  6                delete_session             Can delete session                            
24  6                view_session               Can view session                              
25  7                add_seminar                Can add seminar                               
26  7                change_seminar             Can change seminar                            
27  7                delete_seminar             Can delete seminar                            
28  7                view_seminar               Can view seminar                              
29  7                accountant                 Can view all seminars regardless of assignment
30  8                add_event                  Can add event                                 
31  8                change_event               Can change event                              
32  8                delete_event               Can delete event                              
33  8                view_event                 Can view event                                
34  9                add_record                 Can add record                                
35  9                change_record              Can change record                             
36  9                delete_record              Can delete record                             
37  9                view_record                Can view record                               
38  10               add_banktransaction        Can add bank transaction                      
39  10               change_banktransaction     Can change bank transaction                   
40  10               delete_banktransaction     Can delete bank transaction                   
41  10               view_banktransaction       Can view bank transaction                     
42  11               add_virtualtransaction     Can add virtual transaction                   
43  11               change_virtualtransaction  Can change virtual transaction                
44  11               delete_virtualtransaction  Can delete virtual transaction                
45  11               view_virtualtransaction    Can view virtual transaction                  
46  12               add_invoice                Can add invoice                               
47  12               change_invoice             Can change invoice                            
48  12               delete_invoice             Can delete invoice                            
49  12               view_invoice               Can view invoice                              
50  13               add_invoiceitem            Can add invoice item                          
51  13               change_invoiceitem         Can change invoice item                       
52  13               delete_invoiceitem         Can delete invoice item                       
53  13               view_invoiceitem           Can view invoice item                         
54  14               add_client                 Can add client                                
55  14               change_client              Can change client                             
56  14               delete_client              Can delete client                             
57  14               view_client                Can view client       
A ještě: systém práv je teď v aplikaci hrozně překomplikovaný, protože Django si tam dogenerovává tunu vlastních práv, která ale nikde nepoužíváme. Jestli dobře počítám, tak jich je přes 50 různých (třeba `budget.{add,change,delete,view}_{seminar,event,record,banktransaction,virtualtransaction}` + podobně rozsáhlé sety na `invoicer.` a `auth.`). Bylo by pěkné systém práv nějak rozumně popsat, zbavit se tohohle hrozného balastu automatických práv a používat jenom naše práva. Přemýšlím, jestli pak nedává smysl mít práva scoped = můžeš mít právo `accountant(*)` nebo třeba jenom `accountant(KSP)` a mít to všechno navázané na tohle 🤷 Ale to by chtělo asi nejdřív zkusit někam sepsat. Současná práva viz dotaz do SQLite: ``` sqlite> SELECT * FROM auth_permission; id content_type_id codename name -- --------------- ------------------------- ---------------------------------------------- 1 1 add_logentry Can add log entry 2 1 change_logentry Can change log entry 3 1 delete_logentry Can delete log entry 4 1 view_logentry Can view log entry 5 2 add_permission Can add permission 6 2 change_permission Can change permission 7 2 delete_permission Can delete permission 8 2 view_permission Can view permission 9 3 add_group Can add group 10 3 change_group Can change group 11 3 delete_group Can delete group 12 3 view_group Can view group 13 4 add_user Can add user 14 4 change_user Can change user 15 4 delete_user Can delete user 16 4 view_user Can view user 17 5 add_contenttype Can add content type 18 5 change_contenttype Can change content type 19 5 delete_contenttype Can delete content type 20 5 view_contenttype Can view content type 21 6 add_session Can add session 22 6 change_session Can change session 23 6 delete_session Can delete session 24 6 view_session Can view session 25 7 add_seminar Can add seminar 26 7 change_seminar Can change seminar 27 7 delete_seminar Can delete seminar 28 7 view_seminar Can view seminar 29 7 accountant Can view all seminars regardless of assignment 30 8 add_event Can add event 31 8 change_event Can change event 32 8 delete_event Can delete event 33 8 view_event Can view event 34 9 add_record Can add record 35 9 change_record Can change record 36 9 delete_record Can delete record 37 9 view_record Can view record 38 10 add_banktransaction Can add bank transaction 39 10 change_banktransaction Can change bank transaction 40 10 delete_banktransaction Can delete bank transaction 41 10 view_banktransaction Can view bank transaction 42 11 add_virtualtransaction Can add virtual transaction 43 11 change_virtualtransaction Can change virtual transaction 44 11 delete_virtualtransaction Can delete virtual transaction 45 11 view_virtualtransaction Can view virtual transaction 46 12 add_invoice Can add invoice 47 12 change_invoice Can change invoice 48 12 delete_invoice Can delete invoice 49 12 view_invoice Can view invoice 50 13 add_invoiceitem Can add invoice item 51 13 change_invoiceitem Can change invoice item 52 13 delete_invoiceitem Can delete invoice item 53 13 view_invoiceitem Can view invoice item 54 14 add_client Can add client 55 14 change_client Can change client 56 14 delete_client Can delete client 57 14 view_client Can view client ```
This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feature/auth-hardening:feature/auth-hardening
git switch feature/auth-hardening
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
wikiker/helenka!24
No description provided.