ModularRealmAuthorizer isPermitted implementation with multiple permissions to check

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

ModularRealmAuthorizer isPermitted implementation with multiple permissions to check

Modanese, Riccardo
Hi all, 

   I have a question about the ModularRealmAuthorizer implementation (Shiro version 1.3.2).
There are 2 methods to check multiple permissions:
  public boolean[] isPermitted(PrincipalCollection principals, String... permissions)
  public boolean[] isPermitted(PrincipalCollection principals, List<Permission> permissions)

Both of these implementations does a loop to call the isPermitted method with a single permission.
So the AuthorizingRealm method doGetAuthorizationInfo is called at each iteration. (we aren’t using cache)

Since the AuthorizingRealm has a specific implementation for the isPermitted method with multiple permissions we tried to use it customizing the ModularRealmAuthorizer.
In Kapua project we wrote a custom ModularRealmAuthorizer implementation (see [1]) to reduce the doGetAuthorizationInfo calls count (with the "at least one realm” as result aggregation strategy).

In the ModularRealmAuthorizer did you implement the isPermitted method with the for loop to use the realm aggregation strategy configuration for the realms results?
If not, is it possible to change the implementation to make it more performant (avoiding multiple doGetAuthorizationInfo)?

Thank you

Riccardo

[1] https://github.com/eclipse/kapua/blob/develop/broker-core/src/main/java/org/eclipse/kapua/broker/core/security/EnhModularRealmAuthorizer.java#L47
Reply | Threaded
Open this post in threaded view
|

Re: ModularRealmAuthorizer isPermitted implementation with multiple permissions to check

Benjamin Marwell
I think you "just" changed the loop:

The current ModularRealmAuthorizer checks:

boolean permission[]
For every permission
   for every realm
      permission[i] = isPermitted

But your loop does:

boolean permission[]
For every realm
   for every permission
     permission[i] = isPermitted
     if (permission = true); break

i.e. changing the loop enables to shortcircuit.

Additionally: In every case, we could skip those which are already
permitted (i.e. set to true):

for every permission
  if (permission[i] = true); continue

Did I get this right?

Am Mo., 30. März 2020 um 08:52 Uhr schrieb Modanese, Riccardo
<[hidden email]>:

>
> Hi all,
>
>    I have a question about the ModularRealmAuthorizer implementation (Shiro version 1.3.2).
> There are 2 methods to check multiple permissions:
>   public boolean[] isPermitted(PrincipalCollection principals, String... permissions)
>   public boolean[] isPermitted(PrincipalCollection principals, List<Permission> permissions)
>
> Both of these implementations does a loop to call the isPermitted method with a single permission.
> So the AuthorizingRealm method doGetAuthorizationInfo is called at each iteration. (we aren’t using cache)
>
> Since the AuthorizingRealm has a specific implementation for the isPermitted method with multiple permissions we tried to use it customizing the ModularRealmAuthorizer.
> In Kapua project we wrote a custom ModularRealmAuthorizer implementation (see [1]) to reduce the doGetAuthorizationInfo calls count (with the "at least one realm” as result aggregation strategy).
>
> In the ModularRealmAuthorizer did you implement the isPermitted method with the for loop to use the realm aggregation strategy configuration for the realms results?
> If not, is it possible to change the implementation to make it more performant (avoiding multiple doGetAuthorizationInfo)?
>
> Thank you
>
> Riccardo
>
> [1] https://github.com/eclipse/kapua/blob/develop/broker-core/src/main/java/org/eclipse/kapua/broker/core/security/EnhModularRealmAuthorizer.java#L47
Reply | Threaded
Open this post in threaded view
|

Re: ModularRealmAuthorizer isPermitted implementation with multiple permissions to check

Modanese, Riccardo
I agree with your analysis.

The goal is if there is a way to avoid multiple calls to the doGetAuthorizationInfo (at least with our use case).
So changing the loop can avoid too many calls since, regardless of the permissions checked, there is just one call per realm. On the other hand, if there are few realms, as you said, the risk is to execute checks also if the result is already determined.

Then, assuming to have one realm, do you think our solution could be right?

> Il giorno 30 mar 2020, alle ore 12:35, Benjamin Marwell <[hidden email]> ha scritto:
>
> I think you "just" changed the loop:
>
> The current ModularRealmAuthorizer checks:
>
> boolean permission[]
> For every permission
>   for every realm
>      permission[i] = isPermitted
>
> But your loop does:
>
> boolean permission[]
> For every realm
>   for every permission
>     permission[i] = isPermitted
>     if (permission = true); break
>
> i.e. changing the loop enables to shortcircuit.
>
> Additionally: In every case, we could skip those which are already
> permitted (i.e. set to true):
>
> for every permission
>  if (permission[i] = true); continue
>
> Did I get this right?
>
> Am Mo., 30. März 2020 um 08:52 Uhr schrieb Modanese, Riccardo
> <[hidden email]>:
>>
>> Hi all,
>>
>>   I have a question about the ModularRealmAuthorizer implementation (Shiro version 1.3.2).
>> There are 2 methods to check multiple permissions:
>>  public boolean[] isPermitted(PrincipalCollection principals, String... permissions)
>>  public boolean[] isPermitted(PrincipalCollection principals, List<Permission> permissions)
>>
>> Both of these implementations does a loop to call the isPermitted method with a single permission.
>> So the AuthorizingRealm method doGetAuthorizationInfo is called at each iteration. (we aren’t using cache)
>>
>> Since the AuthorizingRealm has a specific implementation for the isPermitted method with multiple permissions we tried to use it customizing the ModularRealmAuthorizer.
>> In Kapua project we wrote a custom ModularRealmAuthorizer implementation (see [1]) to reduce the doGetAuthorizationInfo calls count (with the "at least one realm” as result aggregation strategy).
>>
>> In the ModularRealmAuthorizer did you implement the isPermitted method with the for loop to use the realm aggregation strategy configuration for the realms results?
>> If not, is it possible to change the implementation to make it more performant (avoiding multiple doGetAuthorizationInfo)?
>>
>> Thank you
>>
>> Riccardo
>>
>> [1] https://github.com/eclipse/kapua/blob/develop/broker-core/src/main/java/org/eclipse/kapua/broker/core/security/EnhModularRealmAuthorizer.java#L47

Reply | Threaded
Open this post in threaded view
|

Re: ModularRealmAuthorizer isPermitted implementation with multiple permissions to check

Brian Demers
+1

It does look like there is some optimization we could do here.  Even when there is multiple realms, we could check only the "failed" permissions on each subsequent realm.
Same for `isPermittedAll` and any of the role or permission checks that take an array/collection.

Thoughts?




On Tue, Mar 31, 2020 at 4:49 AM Modanese, Riccardo <[hidden email]> wrote:
I agree with your analysis.

The goal is if there is a way to avoid multiple calls to the doGetAuthorizationInfo (at least with our use case).
So changing the loop can avoid too many calls since, regardless of the permissions checked, there is just one call per realm. On the other hand, if there are few realms, as you said, the risk is to execute checks also if the result is already determined.

Then, assuming to have one realm, do you think our solution could be right?

> Il giorno 30 mar 2020, alle ore 12:35, Benjamin Marwell <[hidden email]> ha scritto:
>
> I think you "just" changed the loop:
>
> The current ModularRealmAuthorizer checks:
>
> boolean permission[]
> For every permission
>   for every realm
>      permission[i] = isPermitted
>
> But your loop does:
>
> boolean permission[]
> For every realm
>   for every permission
>     permission[i] = isPermitted
>     if (permission = true); break
>
> i.e. changing the loop enables to shortcircuit.
>
> Additionally: In every case, we could skip those which are already
> permitted (i.e. set to true):
>
> for every permission
>  if (permission[i] = true); continue
>
> Did I get this right?
>
> Am Mo., 30. März 2020 um 08:52 Uhr schrieb Modanese, Riccardo
> <[hidden email]>:
>>
>> Hi all,
>>
>>   I have a question about the ModularRealmAuthorizer implementation (Shiro version 1.3.2).
>> There are 2 methods to check multiple permissions:
>>  public boolean[] isPermitted(PrincipalCollection principals, String... permissions)
>>  public boolean[] isPermitted(PrincipalCollection principals, List<Permission> permissions)
>>
>> Both of these implementations does a loop to call the isPermitted method with a single permission.
>> So the AuthorizingRealm method doGetAuthorizationInfo is called at each iteration. (we aren’t using cache)
>>
>> Since the AuthorizingRealm has a specific implementation for the isPermitted method with multiple permissions we tried to use it customizing the ModularRealmAuthorizer.
>> In Kapua project we wrote a custom ModularRealmAuthorizer implementation (see [1]) to reduce the doGetAuthorizationInfo calls count (with the "at least one realm” as result aggregation strategy).
>>
>> In the ModularRealmAuthorizer did you implement the isPermitted method with the for loop to use the realm aggregation strategy configuration for the realms results?
>> If not, is it possible to change the implementation to make it more performant (avoiding multiple doGetAuthorizationInfo)?
>>
>> Thank you
>>
>> Riccardo
>>
>> [1] https://github.com/eclipse/kapua/blob/develop/broker-core/src/main/java/org/eclipse/kapua/broker/core/security/EnhModularRealmAuthorizer.java#L47

Reply | Threaded
Open this post in threaded view
|

Re: ModularRealmAuthorizer isPermitted implementation with multiple permissions to check

Benjamin Marwell
>  we could check only the "failed" permissions on each subsequent realm.

Yes, had the same idea.
But there is a caveat: imagine one of your realms is a database realm
which will only to Authorization, and you have an LDAP realm which
will do both Authz and authc. If caching is not enabled, it would be
very expensive to ask both realms three times (for three permissions).

If the public API permits it, it would be better to first go
realm-by-realm, then go for each permission which is not yet set to
true.

Btw, the shiro code could use some comments. I wasn't aware that a
boolean[] is automatically OR'ed.

Do we have an issue for this? => https://issues.apache.org/jira/

Am Di., 31. März 2020 um 17:05 Uhr schrieb Brian Demers
<[hidden email]>:

>
> +1
>
> It does look like there is some optimization we could do here.  Even when there is multiple realms, we could check only the "failed" permissions on each subsequent realm.
> Same for `isPermittedAll` and any of the role or permission checks that take an array/collection.
>
> Thoughts?
>
>
>
>
> On Tue, Mar 31, 2020 at 4:49 AM Modanese, Riccardo <[hidden email]> wrote:
>>
>> I agree with your analysis.
>>
>> The goal is if there is a way to avoid multiple calls to the doGetAuthorizationInfo (at least with our use case).
>> So changing the loop can avoid too many calls since, regardless of the permissions checked, there is just one call per realm. On the other hand, if there are few realms, as you said, the risk is to execute checks also if the result is already determined.
>>
>> Then, assuming to have one realm, do you think our solution could be right?
>>
>> > Il giorno 30 mar 2020, alle ore 12:35, Benjamin Marwell <[hidden email]> ha scritto:
>> >
>> > I think you "just" changed the loop:
>> >
>> > The current ModularRealmAuthorizer checks:
>> >
>> > boolean permission[]
>> > For every permission
>> >   for every realm
>> >      permission[i] = isPermitted
>> >
>> > But your loop does:
>> >
>> > boolean permission[]
>> > For every realm
>> >   for every permission
>> >     permission[i] = isPermitted
>> >     if (permission = true); break
>> >
>> > i.e. changing the loop enables to shortcircuit.
>> >
>> > Additionally: In every case, we could skip those which are already
>> > permitted (i.e. set to true):
>> >
>> > for every permission
>> >  if (permission[i] = true); continue
>> >
>> > Did I get this right?
>> >
>> > Am Mo., 30. März 2020 um 08:52 Uhr schrieb Modanese, Riccardo
>> > <[hidden email]>:
>> >>
>> >> Hi all,
>> >>
>> >>   I have a question about the ModularRealmAuthorizer implementation (Shiro version 1.3.2).
>> >> There are 2 methods to check multiple permissions:
>> >>  public boolean[] isPermitted(PrincipalCollection principals, String... permissions)
>> >>  public boolean[] isPermitted(PrincipalCollection principals, List<Permission> permissions)
>> >>
>> >> Both of these implementations does a loop to call the isPermitted method with a single permission.
>> >> So the AuthorizingRealm method doGetAuthorizationInfo is called at each iteration. (we aren’t using cache)
>> >>
>> >> Since the AuthorizingRealm has a specific implementation for the isPermitted method with multiple permissions we tried to use it customizing the ModularRealmAuthorizer.
>> >> In Kapua project we wrote a custom ModularRealmAuthorizer implementation (see [1]) to reduce the doGetAuthorizationInfo calls count (with the "at least one realm” as result aggregation strategy).
>> >>
>> >> In the ModularRealmAuthorizer did you implement the isPermitted method with the for loop to use the realm aggregation strategy configuration for the realms results?
>> >> If not, is it possible to change the implementation to make it more performant (avoiding multiple doGetAuthorizationInfo)?
>> >>
>> >> Thank you
>> >>
>> >> Riccardo
>> >>
>> >> [1] https://github.com/eclipse/kapua/blob/develop/broker-core/src/main/java/org/eclipse/kapua/broker/core/security/EnhModularRealmAuthorizer.java#L47
>>
Reply | Threaded
Open this post in threaded view
|

Re: ModularRealmAuthorizer isPermitted implementation with multiple permissions to check

Brian Demers
If the public API permits it, it would be better to first go
realm-by-realm, then go for each permission which is not yet set to
true.

Agreed! 

Btw, the shiro code could use some comments. I wasn't aware that a
boolean[] is automatically OR'ed.

Do we have an issue for this? => https://issues.apache.org/jira/

Not that I know of, do you or Riccardo want to create one? 
Reply | Threaded
Open this post in threaded view
|

Re: ModularRealmAuthorizer isPermitted implementation with multiple permissions to check

Modanese, Riccardo
I’m not too familiar with Shiro code but I tried to implement the changes in a draft [1].
I’m not too confident about the changes I did in the visibility of few methods and also on relying on a specific Authorization implementation methods (AuthorizingRealm)




Il giorno 31 mar 2020, alle ore 19:27, Brian Demers <[hidden email]> ha scritto:

If the public API permits it, it would be better to first go
realm-by-realm, then go for each permission which is not yet set to
true.

Agreed! 

Btw, the shiro code could use some comments. I wasn't aware that a
boolean[] is automatically OR'ed.

Do we have an issue for this? => https://issues.apache.org/jira/

Not that I know of, do you or Riccardo want to create one? 

Reply | Threaded
Open this post in threaded view
|

Re: ModularRealmAuthorizer isPermitted implementation with multiple permissions to check

Benjamin Marwell
https://issues.apache.org/jira/browse/SHIRO-752

Am Mi., 1. Apr. 2020 um 12:37 Uhr schrieb Modanese, Riccardo
<[hidden email]>:

>
> I’m not too familiar with Shiro code but I tried to implement the changes in a draft [1].
> I’m not too confident about the changes I did in the visibility of few methods and also on relying on a specific Authorization implementation methods (AuthorizingRealm)
>
>
> [1] https://github.com/riccardomodanese/shiro/commit/7b60f8be0a599dc5975e9a4b12f277a3475fdc93
>
>
> Il giorno 31 mar 2020, alle ore 19:27, Brian Demers <[hidden email]> ha scritto:
>
>> If the public API permits it, it would be better to first go
>> realm-by-realm, then go for each permission which is not yet set to
>> true.
>
>
> Agreed!
>>
>>
>> Btw, the shiro code could use some comments. I wasn't aware that a
>> boolean[] is automatically OR'ed.
>>
>> Do we have an issue for this? => https://issues.apache.org/jira/
>>
> Not that I know of, do you or Riccardo want to create one?
>
>