Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Security: IAuthorizator needs IIdentity #941

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Security: IAuthorizator needs IIdentity #941

wants to merge 6 commits into from

Conversation

enumag
Copy link
Contributor

@enumag enumag commented Jan 28, 2013

See the RFC on Nette forum (czech only).

@fprochazka
Copy link
Contributor

👍 Chtělo by to pár testů :)

@enumag
Copy link
Contributor Author

enumag commented Jan 28, 2013

@hosiplan: Ono v podstatě stačí upravit stávající testy, ale to bude chvíli trvat. Mezitím si pls přečti RFC. :-)

hrach
hrach reviewed Jan 28, 2013
View reviewed changes
Nette/Security/Permission.php Outdated
@@ -644,68 +639,66 @@ public function isAllowed($role = self::ALL, $resource = self::ALL, $privilege =
$this->checkResource($resource);
}

foreach ($identity->getRoles() as $role) {
$this->checkRole($role);
if (NULL !== ($result = $this->isRoleAllowed($role, $resource, $privilege))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skaredy coding style, takovyto nikde nevyuzivame :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V Permission je použitý hned na několika místech. Sice se mi to nelíbilo, ale nechci se pouštět do kompletní refaktorizace kódu Permission.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ajo, to je proto, že to David kopiroval z nějakyho jinyho frameworku :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mas to v hlavicce... je to ze zendu :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ta implementace Permission se mi celkově moc nelíbí. Vlastně bych celou tu třídu nejradši vyhodil do addons. :-D

@Majkl578
Copy link
Contributor

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

Přesunuto k diskusi na fórum.

Majkl578
Majkl578 reviewed Jan 30, 2013
View reviewed changes
Nette/Security/Permission.php Outdated
@@ -644,68 +649,75 @@ public function isAllowed($role = self::ALL, $resource = self::ALL, $privilege =
$this->checkResource($resource);
}

if (!$identity) { // quest
$roles = array($this->questRole);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you even test it? This wouldn't work at all, there is no questRole, only guestRole (see @tomaswindsor's comment above).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope I did not test it yet.

@enumag
Copy link
Contributor Author

enumag commented Feb 7, 2013

Please review.

hrach
hrach reviewed Feb 7, 2013
View reviewed changes
tests/Nette/Security/Permission.IsAllowedNonExistent.phpt Outdated
@@ -16,8 +18,9 @@ require __DIR__ . '/../bootstrap.php';


Assert::exception(function() {
$identity = new Identity(1, array('nonexistent'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ups thanks :-)

@dg
Copy link
Member

dg commented Mar 11, 2013

Ještě jiný směr, kam by se to mohlo ubírat: http://forum.nette.org/cs/13458-security-iauthorizator-a-identita#p99180

@enumag
Copy link
Contributor Author

enumag commented Jun 26, 2013

@dg Even if the final implementation will be different, the first commit should be merged.

This is taking as long as I was afraid it would so I've already implemented this as an extension.

@redwormik
Copy link

Hi there! I see two small User-related BC breaks:

  1. Logged in User which was given NULL identity by IAuthenticator (rare, I know) is now seen as logged out.
  2. getRoles() (and isInRole()) used to reflect whether the user was logged in, now they can use roles from IIdentity of logged out user.

@enumag
Copy link
Contributor Author

enumag commented Jul 10, 2013

@redwormik

  1. That's a bug. The login method should throw an exception if NULL was returned by the IAuthenticator.
  2. I didn't like the inconsistency that User::getRoles() and Identity::getRoles() had different return values but now that I think about it again it's probably better.

This pull request is probably not going to be merged so it's useless to push the fix here. I will fix both in my extension though.

@dg dg removed this from the 2.2 milestone Jun 19, 2014
@dg dg force-pushed the master branch 3 times, most recently from 991ba1a to e23de7a Compare August 26, 2014 15:04
@dg dg force-pushed the master branch 2 times, most recently from 489cca2 to 0b969cd Compare May 10, 2018 20:14
@dg dg force-pushed the master branch 2 times, most recently from 09a7d92 to b9698a8 Compare February 10, 2019 23:23
@dg dg force-pushed the master branch 3 times, most recently from 5a8c108 to 3aa3147 Compare February 26, 2019 14:39
@dg dg force-pushed the master branch 2 times, most recently from 5feee0e to 3fc1e40 Compare January 5, 2021 15:23
@dg dg force-pushed the master branch 5 times, most recently from 688f189 to 1bc9d13 Compare January 13, 2023 04:18
@dg dg force-pushed the master branch 2 times, most recently from 7215ae6 to 71b2047 Compare February 12, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
2-application 3-is-pullrequest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@enumag @fprochazka @Majkl578 @dg @redwormik @rostenkowski @hrach @tomaswindsor