Refactoring Presenter::checkRequirements na dve metody

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

Nelibi se mi metoda Presenter::checkRequirements(), ktera, jak rika jeji nazev, ma kontrolovat opravneni/pozadavky. Pokud tyto pozadavky nesedi, mela by vratit „ne“, pripadne s nejakym dalsim vysvetlenim.

V soucasne dobe zakladni implementace v Nette v pripadne ne vyhazuje vyjimku, vetsina projektu (napr Kdyby, puvodni Nella) si pak metodu checkRequrements pretezuje a provadi v ni presmerovani na SignIn presenter. Metoda tedy nic neoveruje, ackoliv to jeji jmeno rika, ale vynucuje.

Problem: metodu tedy neni mozne pouzit pouze pro overeni, protoze me posle do pryc (napr. kdyz pred zobrazenim odkazu chci zjistit, zda na cilovou akci ma uzivatel opravneni).

Reseni: rozdelit metodu na dve casti:
1/ checkRequirements, ktera vraci bool
2/ ensureRequirements, ktera pouze zavola checkRequirements a vyhodi vyjimku/presmeruje

Pak bude mozne si v aplikaci pekne znovupouzivat checkRequrements napr. kdyz se budu rozhodovat, ktere odkazy v sablone zobrazit (ale o tom az nekdy priste…)

Editoval juzna.cz (17. 3. 2012 22:51)

Patrik Votoček
Člen | 2221
+
0
-

proč jsi to rovnou neposlal jako pull?

juzna.cz
Člen | 248
+
0
-

Cekal jsem k tomu nejake komentare od tebe a HosipLana.

Jak se divam na muj kod (zatim closed) a Kdyby, tak by se asi hodilo, aby checkRequirements uz rovnou mohlo vracet informaci o chybe, at se stejne kontroly nemuseji provadet 2×. ensureRequirements by pak tyto informace mohlo pouzit k vytvoreni vyjimky.

Pull poslu za chvili, jen co to dodelam a napisu testy.

Filip Procházka
Moderator | 4668
+
0
-

Otočil bych podmínku (nevznikne bc break)

if ($this->checkRequirements($element) === FALSE) {
	throw new Nette\Application\ForbiddenRequestException;
}

jinak líbí.

Editoval HosipLan (17. 3. 2012 22:43)

juzna.cz
Člen | 248
+
0
-

HosipLan napsal(a):

Otočil bych podmínku

Ta podminka je tak udelana schvalne, aby ti checkRequirements mohlo vratit napr popis chyby (!== TRUE), ktery nasledne v ensureRequirements nacpes do vyjimky.

Filip Procházka
Moderator | 4668
+
0
-

To smrdí chybovými kódy… pokud potřebuješ popisovat chyby, tak rovnou házej výjimku.

Editoval HosipLan (17. 3. 2012 22:46)

juzna.cz
Člen | 248
+
0
-

Jeste poznamka k navratovemu typu checkRequirements v pripade neuspechu. Mame prakticky dve moznosti:

1/ Ruznorode typy (@return bool|mixed, jak je v pullu)
false jako jednoduche hlaseni problemu, pokud staci
string e.g. Required role admin
array pro detailnejsi popis …

To pak zavisi na prislusne aplikaci, jak moc chce mit clovek popsane chyby.

2/ Pouze bool (@return bool):
Dovolime jen true/false, pak se ale tezko budou resit chybove hlasky (viz vyse)

Nox
Člen | 378
+
0
-

Jestli dostaneme popis chyby jako výsledek nebo výjimku není pro zpracovatele až takový rozdíl, ne? Takže bych byl asi pro výjimku

Tento mechanismus je uplatněný např. ve výchozím Authenticatoru v sandboxu.

juzna.cz
Člen | 248
+
0
-

Ono vytvoreni vyjimky neni AFAIK tak levna operace, protoze se musi zkopirovat cely stacktrace. Navic, jak uz nazev sam vypovida, je to vyjimka a tedy pro vyjimecne situace.

Problem to muze zpusobit pokud si napr budu napr testovat hodne metod, pak uz muze hodne vyjimek zabrat opravdu hodne casu.

David Grudl
Nette Core | 7466
+
0
-

Mixování několika druhů návratových hodnot je velmi nešťastné. A že vytvoření výjimky není levná operace se dá označit za předčasnou optimalizaci ve chvíli, kdy je potřeba kvůli testu vyrábět celý presenter.

Celá věc tedy vyžaduje trošku větší analýzu. Implementace by přitom neměla presentery zesložiťovat a počítat s tím, že presenter nemusí být nutně potomkem UI\Presenter.

juzna.cz
Člen | 248
+
0
-

Ve vysledku bych si udelal stejne checkRequirements refactoroval do jednoho authorizatoru spolecneho pro celou aplikaci, a checkRequirements by delegovalo volani na nej.
Pro jednodche pripady bych se tedy ptal primo presenteru (sama sebe); v pripade, ze bych se chtel ptat hodne, tak uz onoho autorizatoru.

juzna.cz
Člen | 248
+
0
-

Tak se jeste zeptam na dve uzce souvisejici otazky:

  1. co delate vy v checkRequirements, kdyz pristup neni povolen?
  2. jak obecne resite, kdyz pristup neni povolen? Nechat vyjimku ForbiddenRequestException nezachycenou asi nebude nejlepsi reseni. Kde presmerujete na Sign:in?

Jak koukam na HosipLanovo Kdyby, tak on v checkRequirements provadi presmerovani, coz vyhodi AbortException (a jako vedlejsi efekt nastavi Presenter::$response, pripadne upravi Presenter::$payload). Ja to delam uplne stejne a Nella take. Delate to vy jinak?

juzna.cz
Člen | 248
+
0
-

Upravil jsem pull request, aby checkAuthorization vracelo pouze bool, je to tedy daleko cistejsi. Presmerovani na Sign:in bych teda ted delal v ensureRequirements. Je to „spravne“?

David Grudl
Nette Core | 7466
+
0
-

juzna.cz napsal(a):

Tak se jeste zeptam na dve uzce souvisejici otazky:

  1. co delate vy v checkRequirements, kdyz pristup neni povolen?

Tohle je přesně důvod, proč jsem to do verze 2.0 neřešil. Neexistuje nic jako $application->signInPresenter;

  1. jak obecne resite, kdyz pristup neni povolen? Nechat vyjimku ForbiddenRequestException nezachycenou asi nebude nejlepsi reseni. Kde presmerujete na Sign:in?
  • ForbiddenRequestException znamená: jsi přihlášen, ale nemáš oprávnění
  • Redirect na Sign:in zamená: nejsi přihlášen
Vojtěch Dobeš
Gold Partner | 1316
+
0
-

Jen pro přehled: ForbiddenRequestException tedy stojí defacto na pozici neexistující AuthorizationException. Je to tak?

David Grudl
Nette Core | 7466
+
0
-

V podstatě ano, ale jde o „Http exception“, která odpovídá chybovému kódu 403.

juzna.cz
Člen | 248
+
0
-

David Grudl napsal(a):

Tohle je přesně důvod, proč jsem to do verze 2.0 neřešil. Neexistuje nic jako $application->signInPresenter;

Presmerovani na Sign:in bych urcite nedelal do Nette, ale bylo by v mojem BasePresenter::ensureRequrements. Muj BasePresenter by pak vypadal asi takto.

public function checkRequirements($element) {
	if (...) return false; // moje kontroly (actionX, signalX, renderX, ...)
	// napr:
	if (!$this->authorizator->isMethodAllowed($this->user, $this, $this->formatActionMethod(...)) return false;

	return true;
}

public function ensureRequirements($element) {
	$ret = $this->checkRequirements($element);

	if(!$ret) {
		if($this->user->loggedIn) throw new ForbiddenRequestException; // already loggen in, but not allowed
		else $this->redirect($this->loginLink); // not logged in -> go to login form
	}
}

Tam, kde se ocekava, ze uzivatel ma pristup (napr tryCall v Presenter::run), se vola ensureRequirements, ktere zajisti, aby byly requirementy splneny. To bud jde tak, ze se uzivatel prihlasi, nebo nejde a vyhodi se vyjimka a request skonci 403 chybou.

Kdyz chci zkontrolovat, zda nekam ma opravneni (napr kontroluju zda zobrazit nejaky odkaz), tak zavolam checkRequirements a ocekavam true/false a nebudu chytat vyjimky. Protoze jde o kontrolu a ta muze bezne rici „ne“, coz neni vyjimecny stav.

Editoval juzna.cz (27. 3. 2012 1:12)

Patrik Votoček
Člen | 2221
+
0
-

v zásadě jediné co je potřeba je mít už v presenteru možnost jednoduše zachytit ForbiddenRequestException.

EDIT: Tak mě napadá co nějakej callback? onForbiddenRequest nebo tak něco?

juzna.cz
Člen | 248
+
0
-

@Vrtak to uz jsi ale odbehl od puvodniho tematu – moznost si overit splneni pozadavku (checkRequirements) bez side-efektu.

Proc vubec v presenteru resit odchytavani vyjimek a tim ho jeste vic komplikovat (kdyz uz ted si porad rikame, ze toho dela strasne moc)?

Jan Tvrdík
Nette guru | 2570
+
0
-

Nevím jak moc to s tím souvisí, ale něco (možná) podobného jsme kdysi vyřešili takto: https://gist.github.com/1545209#L56.