Security: IAuthorizator a identita

Upozornění: Tohle vlákno je hodně staré a informace nemusí být platné pro současné Nette.
enumag
Člen | 2118
+
+3
-

Aktuální stav

Metoda isAllowed rozhraní IAuthorizator dostává jako parametry roli, resource a privilege. Autorizátor poté na jejich základě zjišťuje, zda daná role má dané oprávnění pro danou resource (jak to dělá již záleží na implementaci).

Třída Permission je výchozí implementací tohoto rozhraní. Původní implementace byla později vylepšena o možnost assertačních callbacků (dříve IPermissionAssertion). Tento callback jako parametry dostal objekt třídy Permission a roli, resource a privilege právě ověřovaného pravidla. Nedostal ale roli a resource se kterými byla zavolána metoda isAllowed, která vedla k ověření daného pravidla. Ty si musel zjistit sám pomocí metod $permission->getQueriedRole() a $permission->getQueriedResource().

Pozn. Tyto hodnoty mohly být v principu jiné než parametry, které callback dostal přímo. Jednak kvůli dědičnosti rolí a resource, druhak kvůli možnosti roli a resource při volání isAllowed specifikovat jako objekty typu IRole respektive IResource. Kvůli tomu bylo „nutné“ zavést tyto getQueried* metody.

Co je na tom špatně?

Uvažme, že chceme ověřit některý z následujících případů:

  1. Resource a uživatel splňují určitý vztah (např. vlastnictví). – Lze řešit neohrabaně přes samostatnou roli pro každého uživatele (což jde zcela proti mému chápání rolí) nebo principiálně stejným hackem.
  2. Resource splňuje určité kritérium (např. článek může být zveřejněn pouze po korektuře). – Lze řešit pomocí IResource a getQueriedResource.
  3. Uživatel splňuje určité kritérium (např. uživatel smí mít pouze jednu aktivní žádost a nemůže odeslat další dokud první nebyla vyřízena). – Lze řešit podobně jako případ 1).

Pozn. pokud by provedení akce v případě 2) nebo 3) vedlo k nekonzistentní databázi, musí to být zakázané mimo autorizátor. Je-li to pouze otázka oprávnění a nějaký superadmin může akci provést i tehdy když normální uživatel nemůže, je to záležitost autorizátoru.

Řešení

Předávat autorizátoru celou IIdentity místo role navrhoval @David Grudl už před dvěma lety, jen se to nikomu nechtělo řešit. (Přestože to prakticky všichni považují za nutnou změnu.)

Dále se mi vůbec nelíbí parametry, které dostává assertační callback. Dle mého názoru by tento callback měl řešit výše uvedené případy a neměl by se vůbec zajímat o kontext (role, resource, privilege) kdy se spouští. Jako parametry tedy navrhuji předávat POUZE identitu a queriedResource. Díky tomu tyto callbacky budou nezávislé na autorizátoru, budou tedy i samostatně testovatelné.

Současně mne nenapadají žádné další případy, kdy bych assertační callbacky potřeboval a připadá mi v pořádku omezit je pouze na tyto 3 případy.

Samozřejmě jde o velký BC break, takže je vše nutné důkladně promyslet.

Implementace

Provedené změny:

  1. V nové implementaci tedy má IAuthorizator::isAllowed jako první parametr $identity namísto původní $role.
  2. Rozhraní IRole mi připadá zbytečné, ostranil jsem jej. @achtan ukázal use-case, rozhraní IRole bylo v zájmu zachování kompatibility zachováno.
  3. Assertační callbacky dostávají jako parametry identitu, queriedResource a queriedRole, nic jiného.
  4. Metody getQueried* jsou tedy zbytečné, odstranil jsem je.
  5. V třídách User a Permission jsem opravil kompatibilitu s IAuthorizator.
  6. User spouští autorizaci pouze je-li uživatel přihlášen, identita jinak nemusí existovat nebo nemusí být platná.
  7. User::getIdentity() vrací NULL pokud uživatel není přihlášen
  8. Automatické role guest a authenticated přesunuty do Permission
  9. Autorizátor místo identity dostane NULL pokud uživatel není přihlášen (řeší připomínku od @Jan Tvrdík a zachovává nejlepší možnou zpětnou kompatibilitu)
  10. Opravena kompatibilita testů tříd User a Permission.
  11. Přidány nové testy Permission, které testují guest a authenticated role.

Pull request. Nyní je tam pouze nová implementace, testy opravím časem. Mezitím můžeme vše prodiskutovat. Testy doplněny, prosím o review. :-)

K diskusi

  1. Měl by všechny 3 případy uvedené v „Co je na tom špatně?“ skutečně řešit autorizátor? – Dle mého názoru ano, je-li splněna uvedená poznámka, ale mohu se mýlit.
  2. Napadají vás nějaké případy, které by měl řešit autorizátor pomocí callbacků a neuvedl jsem je? Uveďte v komentářích.
  3. Používáte rozhraní IRole a je ve vašem případě vhodnější než IIdentity? Uveďte v komentářích.
  4. Používáte v assertačním callbacku některý z parametrů role/resource/privilege? – Zamyslete se zda by callback neměl být rozdělen na několik různých funkcí, tak aby nebyly na těchto parametrech závislé. Pokud jste přesvědčeni že ne, uveďte v komentářích.
  5. Připomínky k implementaci? (prosím na GitHub)
  6. Jakékoli další poznatky, něco na co jsem zapomněl…?
  7. Měla by tato změna být zahrnuta ještě do Nette 2.1?
    • Podle mne ano, verze 2.2 se jen tak nedočkáme a všichni kdo s tímto mají problém, ale chtějí používat stable, by tak museli čekat.
    • @Vrtak-cz je toho názoru, že dávat takovýto BC break ještě do 2.1 je trochu pozdě. (lednová posobota)
    • @David Grudl řekl: Proč ne, jednou se to stejně udělat musí. (lednová posobota)

Pozn. Třeba @Jan Tvrdíkněkolik let nepoužívá IAuthorizator, takže by změnu zřejmě rovněž uvítal.

Editoval enumag (1. 2. 2013 20:33)

hrach
Člen | 1838
+
0
-

Šup tam s tím!

Filip Procházka
Moderator | 4668
+
0
-

Krásné RFC, přijatelná implementace. Osobně jako řešení používám co jsi uvedl v bodu 1) jako hack, ale tohle je samozřejmě lepší.

pekelnik
Člen | 462
+
0
-

Pecka.

Jan Tvrdík
Nette guru | 2595
+
0
-

Co se týče vydání, tak já navrhuji vydat 2.1 co nejdřív s minimem BC breaků. Tohle a DI blbosti pak vydat o dva až tři měsíce později jako 2.2.

Co se týče implementace, tak se mi nelíbí, že User::isAllowed vrátí pro nepřihlášeného uživatele vždy false. Zřejmě by kvůli tomu bylo potřeba mít guest identity nebo to řešit úplně jinak. Viz jak to používám.

enumag
Člen | 2118
+
0
-

@Jan Tvrdík:
Vydání 2.1 bez BC breaků záleží na @dg, ale asi to neprojde. Odříznout podporu pro 2.0 je myslím ještě brzy a takto by někdo musel udržovat 3 různé větve.

S druhým odstavcem souhlasím, ale nejsem si jistý, jak to řešit. Guest identita by zřejmě rovnou měla automatickou roli guest, což je trochu problém:

  1. Jak by se autorizátor (permission) dozvěděl že tato role je platná a že nemá vyhazovat výjimku protože tu roli nezná? nijak, dřív se to taky neřešilo
  2. Asi by to znamenalo přepsání identity tou guest identitou hned při odhlášení. (K čemu je vlastně dobré to, že se při odhlášení identita nemaže automaticky (pokud se nezavolá logout(TRUE))?) lze obejít, viz následující komentář
  3. Ještě mne napadá podstrčit autorizátoru falešnou identitu s rolí NULL (=ALL), s tím by ale zase musely počítat jiné implementace. fuj, na to je User::$guestRole

Editoval enumag (1. 2. 2013 23:07)

enumag
Člen | 2118
+
0
-

Díval jsem se podrobněji na implementaci třídy User jak je to s guestRole a authenticatedRole. Zkusil jsem tu logiku z User::getRoles() přesunout do User::getIdentity(), ale moc se mi to nedaří:

private $identity;

public function getIdentity()
{
	if (!$this->isLoggedIn()) {
		//tohle je myslím v pohodě, původní identita je po odhlášení dostupná přes storage
		if ($this->identity === NULL) {
			$this->identity = new Identity(NULL, array($this->guestRole));
		}
		return $this->identity;
	}
	$identity = $this->storage->getIdentity();
	// může zde $identity být NULL? pokud ano, jak to řešit?
	if (!$identity->getRoles()) {
		if ($this->identity === NULL) {
			// tohle se mi vůbec nelíbí, nahrazování identity jen proto že nemá roli je hodně WTF
			$this->identity = new Identity($identity->getId(), array($this->authenticatedRole));
		}
        return $this->identity;
	}
    return $identity;
}

final public function logout($clearIdentity = FALSE)
{
	$this->identity = NULL; //odstranění případné identity s rolí authenticated
	// ...
}

Takže tudy by to nešlo. V podstatě jediný problém je ta automatická authenticatedRole, tohle by si měl řešit authenticator když vytváří identitu. Můj výsledný návrh tedy zní:

  • z výše uvedeného kódu použít jen první část metody getIdentity() s guest rolí (EDIT: místo User::$identity bude lepší User::$guestIdentity)
  • authenticatedRole přesunout do SimpleAuthenticator nebo úplně odstranit, o vlastní implementace IAuthenticator se nestarat, ty si to musí vyřešit samy
  • EDIT: anebo tu roli authenticated může přidávat nettí implementace Identity, ale to se mi nezamlouvá

Prosím o názory!! Tu guestRole a authenticatedRole je nutné nějak vyřešit, ten PR jinak nelze mergnout.

Editoval enumag (1. 2. 2013 23:06)

enumag
Člen | 2118
+
0
-

Názorů jsem se nedočkal, takže jsem to implementoval dle svého návrhu. Snad se nějakých názorů dočkám teď.

Zapomeňte na to, dostal jsem mnohem lepší nápad. Ráno to sepíšu. Sepsáno níže. Omlouvám se za zmatky.

Editoval enumag (30. 1. 2013 2:18)

enumag
Člen | 2118
+
0
-

Mini-RFC: Co s guestRole a authenticatedRole?

Jsou vůbec potřeba?

Mějme obecný autorizátor = implementaci třídy IAuthorizator (pro tuto chvíli zapomeňte na Permission, k tomu se dostanu). Jak autorizátor který dostane identitu rozezná případy „guest“ a „authenticated“?

  • Guest: Stačí autorizátor spouštět vždy, s tím že místo identity může dostat NULL – což bude znamenat právě uživatele bez identity, tedy hosta.
  • Authenticated: Tato role byla uživateli automaticky přidána pokud žádnou neměl. Když ale autorizátor má celou identitu tak to pozná velmi snadno taky, $identity->getRoles() vrátí prázdné pole.

A co Permission?

Řekněme, že chceme například zobrazit reklamy pouze anonymním návštěvníkům. V případě Permission by se nám na to ale opravdu hodila právě role guest. Pointa je, že potřeba guest jako role je specifikum autorizátoru Permission a jiný autorizátor nic takového nepotřebuje (nebo si to vyřeší sám, třeba i podobně jako Permission). Totéž platí i pro rolí authenticated.

Řešením tedy bude přesunout tyto Permission-specific role pěkně do Permission a neprasit tím třídu User, ani nic jiného. Aneb každému co jeho jest.


Implementace na GitHubu (force-pushed do pull requestu).

Editoval enumag (1. 2. 2013 23:07)

hrach
Člen | 1838
+
0
-

Noooo, jestli to dobre chapu, tak nepocitas s pripadem, kdy uzivatel se odhlasil, ale identita zustava.

enumag
Člen | 2118
+
0
-

@hrach: Samozřejmě, že počítám. :-) Řeším to tak, že User::getIdentity() vrátí NULL pokud uživatel není přihlášen.

V prvním příspěvku upraven oddíl implementace.

Editoval enumag (30. 1. 2013 2:16)

hrach
Člen | 1838
+
0
-

No tak ale to je zbytecny BC break. To uz to rovnou identitu ze session muzu mazat uplne, kdyz ji pak nemam jak ziskat, ne?

(Edit: otazka, zda to nekdy nekdo k necemu pouzil, pripadne k necemu jinemu, nez reason odhlaseni)

Editoval hrach (30. 1. 2013 2:23)

Majkl578
Moderator | 1364
+
0
-

Chtěl bych tě poprosit, aby ses naučil psát malé písmeno G. Opravdu to není „q“, nýbrž „g“, tudíž „questIdentity“ (což má mimochodem v překladu podivný význam) má být „guestIdentity“ a tak dále.

Editoval Majkl578 (30. 1. 2013 5:02)

Majkl578
Moderator | 1364
+
0
-

Na GitHubu jsem ještě utrousil tento komentář, budu ho citovat i tady, jelikož tohle je vhodné místo pro diskusi:

Přemýšlím nad tím a ta vazba přímo na IIdentity mi přijde poměrně zbytečná, až nežádoucí.

Říkám si, jestli by nebylo lepší něco obecnějšího, např. IRoleSet nebo IRolesProvider s metodou getRoles(). IIdentity by jej a) extendovalo a zůstalo současné chování, b) neextendovalo a možnost rolí by zůstala plně v moci uživatele (programátora). Druhá varianta by mi nejspíš dávala větší smysl, sám jsem dělal několik projektů, kde jsem role vůbec nepoužíval a musel hloupě vytvářet metodu getRoles vracející prázdné pole. Zároveň by nebyl problém používat i nadále Permission zcela nezávisle na IIdentity/User (jako tomu bylo doposud a což tento pull request znemožňuje).

Editoval Majkl578 (30. 1. 2013 5:02)

enumag
Člen | 2118
+
0
-

@hrach: Stále se dá získat přes storage. Dává mi mnohem lepší smysl, že User::getIdentity() vrací pouze platnou identitu, ale samozřejmě mohu tu podmínku přesunout do User::isAllowed(). Nechť se vyjádří ještě někdo další, takto je to názor proti názoru. ;-) (Že je to možná zbytečný BC break je pravda, ale důvod jsem už napsal.)

@Majkl578: (ad quest) Jistě, psal jsem to po půlnoci a už jsem to sám po sobě nečetl. Fixed. Mimochodem přesně na tyhle detaily slouží komentáře na GitHubu a bohatě stačil ten první comment od @tomaswindsor. Psát to sem bylo vyloženě zbytečné.

@Majkl578: (ad komentář) To považuji za zbytečnou abstrakci. Navíc to vlastně ani nejde, protože metody isAllowed různých autorizátorů by měly různé hlavičky (někdy IIdentity, někdy IRoleSet, poodle toho co potřebuje) což ti ani PHP nedovolí. Už to by ti mohlo jasně ukázat, že na tvém návrhu je něco špatně. Vzhledem k tomu že jediné co bys tím ušetřil je jednořádková metoda getRoles to imho není dobrý nápad.

EDIT:

@Majkl578: Na třídě User Permission nezávislé je. Rozhraní IIdentity je natolik primitivní, že nezávislost není třeba.

@hrach: Opraven coding style v Permission.

Editoval enumag (30. 1. 2013 10:10)

hrach
Člen | 1838
+
0
-

@hrach: Vtip je v tom, že identity v guestovi je platná i po odhlášení. Ten uživatel má prostě identitu, jen se odhlásil. Tedy jeho autorizace je neplatná, ale ne identita. Netvrdím, že tvé řešení nemá smysl, jen se pozastavuji nad touto zmenu, pac je zasadni, a premyslim, jestli je treba vubec jeste toto chovani mit.

enumag
Člen | 2118
+
0
-

@hrach: S tím souhlasím. Když nad tím teď přemýšlím tak mi moc nesedí, že storage->getIdentity vrací něco jiného než getIdentity. Ale stejně bych rád aby se vyjádřil ještě někdo další.

Mini-RFC: Konstanty ALL, ALLOW a DENY

Ještě přemýšlím zda nepřesunout konstanty ALL, ALLOW a DENY z IAuthorizatoru do Permission. Respektive konstanty ALLOW a DENY bych možná zrušil úplně, co znamená TRUE a FALSE je každému jasné. Konstanta ALL mi v IAuthorizatoru nedává smysl.

Důvod: Kdyby třída Permission neexistovala tak by v IAuthorizatoru tyto konstanty asi nikdy nevznikly.

Jo, je to BC break a jo, možná zbytečný. Připadá mi ale, že by to tak bylo lepší. A kdy jindy ten BC break udělat než teď.

EDIT: Koukám že Permission stejně všude používá self takže v tom IAuthorizatoru je to zřejmě jen kvůli 5.2 kompatibilitě. Raději to nechám být.

Editoval enumag (30. 1. 2013 13:15)

enumag
Člen | 2118
+
0
-

@hrach: Asi bude lepší ten getIdentity BC break nedělat, fixnul jsem to.

Implementaci považuji za finální, nyní jsou na řadě testy.

Filip Procházka
Moderator | 4668
+
0
-

Asi by bylo vhodné sepsat seznam use-cases, které Permission (potažmo implementace IAuthorizator) objekt má řešit, předtím, než implementaci „uzavřeš za finální“ :)

David Ďurika
Člen | 328
+
0
-

Jak bych řešil, kdybych použít assertion callbacky pro porovnávání vlastnictví v případě, že by nějaký resource mohla vlastnit i role? Typicky když mám například kategorii článků a chci aby moderátor mohl upravovat jenom tu jednu kategorii.

enumag
Člen | 2118
+
0
-

@Filip Procházka: Nerozumím, use-cases jsou přece stejné jako doteď (na to stačí dokumentace) plus ty případy uvedené v prvním příspěvku. To navíc byly use-cases už dříve, jen se řešily neohrabaně. Tzn. use-cases se nijak nezměnily.

@achtan: Ke každé kategorii budeš mít (asi v db) seznam ID uživatelů, kteří ji mohou upravovat takže assertační callback nějak takto:

function (IIdentity $identity, IResource $category) {
	return in_array($identity->getId(), $category->getModerators());
}

Editoval enumag (30. 1. 2013 14:37)

Filip Procházka
Moderator | 4668
+
0
-

Tak jinak. Jsem zvědavý na testy :)

enumag
Člen | 2118
+
0
-

@Filip Procházka: Jo, to já taky. Hlavně jsem zvědavej kdy na ně najdu čas. :-D

Ještě mi vrtá hlavou to co psal @achtan. Pokud uvedený případ nebudu chtít řešit tak jak jsem napsal výše, ale chtěl bych mít roli „moderátor-kategorie-<id_kategorie>“, tak je to naprosto zřejmý use-case pro rozhraní IRole. Otázka je zda to tak někdo používá a zdá má smysl to podporovat.

A prosím vyjádřete se, zatím všechna rozhodnutí necháváte na mne, což není úplně dobře.

Filip Procházka
Moderator | 4668
+
0
-

Ty to totiž moc hrneš, já třeba nemám vůbec čas nad tím nějak víc přemýšlet a ty už jsi to za jeden den 4× předesignoval, ale chápu, že to chceš mít rychle hotové, aby jsi to mohl používat :)

David Ďurika
Člen | 328
+
0
-

enumag napsal(a):
Otázka je zda to tak někdo používá a zdá má smysl to podporovat.

asi nie… bud by som to spravil tak ako si pisal na zaciatku, alebo by som pri userovy ulozeny zoznam kategorii kt. moze spravovat, urcite by som nerobil rolu pre kazdu kategoriu…

enumag
Člen | 2118
+
0
-

@achtan: Jasně že ne roli pro každou kategorii, ale mohl bys mít roli category-moderator (šlo by o třídu implementující IRole), která by měla jako atribut ID kategorie. Pak bys mohl uživateli přiřadit takovouto roli. Vzhledem k tomu, že to rozhraní IRole v Nette už bylo a tys ukázal use-case ho tam asi nechám.

@Filip Procházka: njn :-D Jeden z důvodů proč to tlačim je, že to chci stihnout ještě do finální 2.1, jakmile se objeví 2.1-alpha (což může být celkem kdykoli) tak už by se tam podobné breaky neměly dostat. Opravim testy a ukážu asi až finální návrh.

enumag
Člen | 2118
+
0
-

Opraveny staré testy + přidány dva nové. Upraven původní příspěvek.

Rozhraní IRole jsem v rámci zachování kompatibility zachoval, odstranit se dá kdykoli a rozhodnutí bych nechal spíše na @dg.

Také jsem poněkud přeházel pořadí commitů, drobné fixy zahrnul do commitů kam patřily, etc.

@Filip Procházka: Předpokládám, že ty testy nyní nejsou to, co jsi po mne chtěl, takže prosím upřesnění. ;-)

Editoval enumag (1. 2. 2013 18:47)

David Grudl
Nette Core | 8227
+
0
-

Díky moc za špičkově zpracované RFC. Zkusím sem hodit ještě pár myšlenek:

Na autorizátor se nemá smysl dívat jako na místo, kde se řeší veškeré autorizační záležitosti – tak to v praxi nefunguje, například z databáze rovnou získáváme záznamy, které může daný uživatel vidět, místo toho, abychom je filtrovali v aplikaci.

Autorizátor z pohledu Nette je tedy něco velmi podobného, jako translátor. Tedy objekt, který předám třeba komponentně DataGrid a on rozhoduje, zda se má určitý prvek zobrazit nebo ne. Proto má také smysl mít pro něj interface. Při takovém použití je ale parametr $role stejně tak i $identity v metodě isAllowed navíc. (A otázkou je, jestli dvojice $resource a $privilege je dostatečně obecná).

Vedle toho tu máte třídu Permission, která zajišťuje správu ACL a pro ni je naopak parameter $role v isAllowed nezbytný. Naopak, předávání Identity věc komplikuje o cyklus, ve kterém se oprávnění kontrolují.

Co z toho vyplývá:

  • můžeme mít nějaký obecný interface ve stylu ITranslator, ale DataGrid si klidně může definovat vlastní
  • Permission by neměl být implementací tohoto interface, místo toho by se použil ještě mezistupeň.
  • autorizátor v tomto pojetí by byl samostatnou jednotkou, User::isAllowed by ztratilo smysl
  • nebyl by žádný problém používat více různých autorizátorů pro různé úkoly
Jan Tvrdík
Nette guru | 2595
+
0
-

@David Grudl: Nice, takhle by to šlo. Vlastně něco velmi podobného už addony používají.

enumag
Člen | 2118
+
0
-

@David Grudl:
Tzn. každý autorizátor (pokud by jich v aplikaci bylo více) by si Identitu (respektive User(a)) musel obstarat přes DI, chápu to správně? To má jednu nevýhodu – takového autorizátoru se nemohu zeptat, zda nějaký uživatel (ne zrovna přihlášený) má oprávnění XYZ, přesněji musel bych vytvářet novou instanci pro každého uživatele anebo používat nějaký setter. A teď si představ že bych chtěl vypsat tabulku uživatelů, kteří jsou oprávněni k provedení určité akce.

Zní to zajímavě, o tom žádná. Uvědom si ale že už tohle RFC čekalo dva roky(!!) než jej někdo sepsal. Jak dlouho by čekalo to co navrhuješ? Dost možná ještě jednou tak dlouho – přinejmenším já na to několik následujících měsíců čas určitě mít nebudu. Dle mého názoru bude lepší do 2.1 dostat alespoň tohle, jinak autorizátor další dva roky zůstane v aktuálním špatném stavu.

Jan Tvrdík
Nette guru | 2595
+
0
-

takového autorizátoru se nemohu zeptat, zda nějaký uživatel (ne zrovna přihlášený) má oprávnění XYZ

Což je činnost, kterou obvykle nepotřebuješ. Hned po tom, co to David napsal jsem prošel kompletně všechna volání isAllowed na signaly.cz a nenašel jsem jediné, kde bychom to volali s někým jiným, než s aktuálně přihlášeným uživatelem.

teď si představ že bych chtěl vypsat tabulku uživatelů, kteří jsou oprávněni k provedení určité akce.

Tohle je věc, kterou je obvykle lepší dělat jinak, než načíst všechny uživatele a po jednom je testovat.

Jak dlouho by čekalo to co navrhuješ?

To, co navrhuje, je výrazně jednodušší na implementaci (aspoň mám ten dojem), než to, cos implementoval ty.

nanuqcz
Člen | 822
+
0
-

Jan Tvrdík napsal(a):

takového autorizátoru se nemohu zeptat, zda nějaký uživatel (ne zrovna přihlášený) má oprávnění XYZ

Což je činnost, kterou obvykle nepotřebuješ.

Ano, důležité je zde to slovo „obvykle“. Protože jednou za čas takový požadavek přijde, kdy to někdo bude potřebovat.