Security: IAuthorizator a identita

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ší.

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.

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)

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: 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)

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)

@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: 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)

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í“ :)

@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: 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.

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…

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: Nice, takhle by to šlo. Vlastně něco velmi podobného už addony používají.

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.