> 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 <[email protected]>: > > +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 > <[email protected]> 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 >> > <[email protected]> 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 >> > <[email protected]>: >> >> >> >> 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 >>
