SimpleRoleGroup#roles

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

SimpleRoleGroup#roles

Philippe Marschall
Hi

I’m aware this may no technically be the right list to discuss this but
this list is impacted by this and fairly active.

During load testing of our application we found a case we spend 10% of
your CPU time in SimpleRole#equals (see attachment). This is because
SimpleRoleGroup uses an ArrayList to maintain a unique set of roles. As
a result it has to call ArrayList#contains a lot, which is itself O(n).
In fact because that’s done when iterating over all the roles it becomes
O(n^2). In our case our principals can have up to 200 roles. I don’t
know if this is exceptionally many or a common case.

We would like to work on a patch but we need your guidance. There are
several possible solutions and it all comes down to whether we can
change the List in the RoleGroup interface to a Collection. All the
users we searched for in WildFly only used the return value for
iterating over, nobody used the index. In fact they all used it an a
for-each loop so the change would even be source compatible but
unfortunately not binary compatible. If we can change the interface then
we can just change the ArrayList in SimpleRoleGroup to a HashSet and be
done with it. If the order is important the we can either use a TreeSet
or a LinkedHashSet.
If changing the RoleGroup interface is not possible then there are two
other possibilities. The first is only internally using a Set but in
getRoles perform a copy. This would produce more garbage. The second
option would be a having a Set and List (to avoid having to do copies)
in SimpleRoleGroup. This would avoid having to do a copy and the Set can
be used for contains checks. Only removeRole would still be O(n) due to
the scan over ArrayList. The only user we found was
OptionsRoleMappingProvider.
All of these would change the serialized form. If it is important to
support reading old instances that could be added as well.

Cheers
Philippe

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev

SimpleRole-equals.png (82K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SimpleRoleGroup#roles

arjan.tijms
Hi,

On Mon, Jun 8, 2015 at 8:25 AM, Philippe Marschall <[hidden email]> wrote:
Hi

I’m aware this may no technically be the right list to discuss this but this list is impacted by this and fairly active.

During load testing of our application we found a case we spend 10% of your CPU time in SimpleRole#equals (see attachment). This is because SimpleRoleGroup uses an ArrayList to maintain a unique set of roles. As a result it has to call ArrayList#contains a lot, which is itself O(n). In fact because that’s done when iterating over all the roles it becomes O(n^2). In our case our principals can have up to 200 roles. I don’t know if this is exceptionally many or a common case.

200 doesn't seem like so many. The "problem" often is that the term "role" makes people think they are only allowed to use it for things like "administrator" and "manager" and such. In that view 200 may be much.

But, nothing in Java EE security imposes this, and if you want to use roles with names like "can_see_updated_interest", then 200 is really nothing.

From the call stack, I guess this is triggered via @RolesAllowed on an EJB right?

Theoretically, this is where JACC could come in according to the Java EE standards. This would allow you to implement your own (optimized if necessary) logic for "is user/caller in role".

However despite being Java EE 7 certified, it seems it's not actually possible to install a JACC provider on JBoss. This is a bit of a spec hole, unfortunately. Most of the code seems to be there in JBoss (WildFly), but there's just no place where you can actually put your own JACC provider.

Kind regards,
Arjan Tijms

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: SimpleRoleGroup#roles

Stefan Guilhen
In reply to this post by Philippe Marschall
The order of the roles is not important. I think we should change the SimpleRole implementation in PicketBox to use Collection (HashSet) reference instead of List and then change any WildFly code that might be using the List reference.

On 06/08/2015 03:25 AM, Philippe Marschall wrote:
Hi

I’m aware this may no technically be the right list to discuss this but this list is impacted by this and fairly active.

During load testing of our application we found a case we spend 10% of your CPU time in SimpleRole#equals (see attachment). This is because SimpleRoleGroup uses an ArrayList to maintain a unique set of roles. As a result it has to call ArrayList#contains a lot, which is itself O(n). In fact because that’s done when iterating over all the roles it becomes O(n^2). In our case our principals can have up to 200 roles. I don’t know if this is exceptionally many or a common case.

We would like to work on a patch but we need your guidance. There are several possible solutions and it all comes down to whether we can change the List in the RoleGroup interface to a Collection. All the users we searched for in WildFly only used the return value for iterating over, nobody used the index. In fact they all used it an a for-each loop so the change would even be source compatible but unfortunately not binary compatible. If we can change the interface then we can just change the ArrayList in SimpleRoleGroup to a HashSet and be done with it. If the order is important the we can either use a TreeSet or a LinkedHashSet.
If changing the RoleGroup interface is not possible then there are two other possibilities. The first is only internally using a Set but in getRoles perform a copy. This would produce more garbage. The second option would be a having a Set and List (to avoid having to do copies) in SimpleRoleGroup. This would avoid having to do a copy and the Set can be used for contains checks. Only removeRole would still be O(n) due to the scan over ArrayList. The only user we found was OptionsRoleMappingProvider.
All of these would change the serialized form. If it is important to support reading old instances that could be added as well.

Cheers
Philippe


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: SimpleRoleGroup#roles

Stefan Guilhen
In reply to this post by arjan.tijms
On 06/08/2015 07:08 AM, arjan tijms wrote:

Theoretically, this is where JACC could come in according to the Java EE standards. This would allow you to implement your own (optimized if necessary) logic for "is user/caller in role".

However despite being Java EE 7 certified, it seems it's not actually possible to install a JACC provider on JBoss. This is a bit of a spec hole, unfortunately. Most of the code seems to be there in JBoss (WildFly), but there's just no place where you can actually put your own JACC provider.

Huh? We would not be certified if we didn't support custom providers as the TCK requires the installation of its own provider. The javax.security.jacc.PolicyConfigurationFactory.provider and javax.security.jacc.policy.provider system properties are both supported by WildFly.


Kind regards,
Arjan Tijms


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: SimpleRoleGroup#roles

arjan.tijms
Hi,

On Mon, Jun 8, 2015 at 3:42 PM, Stefan Guilhen <[hidden email]> wrote:
On 06/08/2015 07:08 AM, arjan tijms wrote:

Theoretically, this is where JACC could come in according to the Java EE standards. This would allow you to implement your own (optimized if necessary) logic for "is user/caller in role".

However despite being Java EE 7 certified, it seems it's not actually possible to install a JACC provider on JBoss. This is a bit of a spec hole, unfortunately. Most of the code seems to be there in JBoss (WildFly), but there's just no place where you can actually put your own JACC provider.

Huh? We would not be certified if we didn't support custom providers as the TCK requires the installation of its own provider. The javax.security.jacc.PolicyConfigurationFactory.provider and javax.security.jacc.policy.provider system properties are both supported by WildFly.

Indeed, you would say that. But I do know that a TCK can be lacking "a little", specifically where it concerns JASPIC and JACC, unfortunately. There are a few other major violations in various certified products. With JASPIC for instance it was possible to certify a product that just didn't implement the actual authentication, which is the core of the core of JASPIC :X

Those properties are of course supported, but where does one put the classes (or jar containing these classes)? I tried for hours at end and asked in the JBoss forum, but it never became clear. The documentation doesn't mention it either. See this for my question about this: https://developer.jboss.org/thread/254106

Would be really cool if the location could become clear. Thanks!

Kind regards,
Arjan Tijms


















 


Kind regards,
Arjan Tijms


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: SimpleRoleGroup#roles

Stefan Guilhen
Arjan,

On 06/08/2015 10:55 AM, arjan tijms wrote:
Hi,

Indeed, you would say that. But I do know that a TCK can be lacking "a little", specifically where it concerns JASPIC and JACC, unfortunately. There are a few other major violations in various certified products. With JASPIC for instance it was possible to certify a product that just didn't implement the actual authentication, which is the core of the core of JASPIC :X

The TCK coverage (or lack thereof) has nothing to do with the ability to configure custom JACC providers. If we didn't support it not a single test of the JACC testsuite would pass so the TCK is not to be blamed in this case. The JASPIC testsuite is another story and I think we both agree that it is broken from our previous conversations. Arun's JEE testsuite, to which you contributed your JASPIC tests, has been much more valuable as a tool to validate the implementation than the TCK itself.

Having said that, the documentation does really seem to be missing a section about custom JACC providers so I went to check the TCK setup. It looks like the TCK JACC providers are bundled in a jar and this jar is being set as a resource of the org.jboss.as.security module. I'm not sure why it was done this way but I believe it should be also possible to define your own module containing the classes and then wire it to the security module as a dependency instead of a resource.


Those properties are of course supported, but where does one put the classes (or jar containing these classes)? I tried for hours at end and asked in the JBoss forum, but it never became clear. The documentation doesn't mention it either. See this for my question about this: https://developer.jboss.org/thread/254106

Would be really cool if the location could become clear. Thanks!

Kind regards,
Arjan Tijms


















 


Kind regards,
Arjan Tijms


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev



_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: SimpleRoleGroup#roles

arjan.tijms
Hi there,

On Mon, Jun 8, 2015 at 8:26 PM, Stefan Guilhen <[hidden email]> wrote:
The TCK coverage (or lack thereof) has nothing to do with the ability to configure custom JACC providers. If we didn't support it not a single test of the JACC testsuite would pass so the TCK is not to be blamed in this case.

Well, one of the requirements of JACC is that the Java EE product has a default JACC provider (which implements the authorization algorithms as defined by both the Servlet and EJB containers). So it could *theoretically* have been the case that the TCK only tests that one. Of course I don't know if this is indeed the case.

JBoss indeed has such a default provider. However, the spec also requires (I think) that in a full Java EE server all authorization decisions (for the APIs defined by Servlet and EJB) go via JACC, which clearly does not happen in JBoss. As far as I know, only GlassFish really does this, while TMaxSoft JEUS comes close.


 
The JASPIC testsuite is another story and I think we both agree that it is broken from our previous conversations. Arun's JEE testsuite, to which you contributed your JASPIC tests, has been much more valuable as a tool to validate the implementation than the TCK itself.

I'm really glad they helped ;)

 

Having said that, the documentation does really seem to be missing a section about custom JACC providers so I went to check the TCK setup. It looks like the TCK JACC providers are bundled in a jar and this jar is being set as a resource of the org.jboss.as.security module. I'm not sure why it was done this way but I believe it should be also possible to define your own module containing the classes and then wire it to the security module as a dependency instead of a resource.

Hmmm, how would one go about doing that exactly? I think I created a module for my custom JACC provider, then set that as a dependency for the security module (since that was the place the default implementation lives), but it again did not work (class not found exceptions). Could well be the case that I did something wrong, so an example would be great.

Kind regards,
Arjan Tijms

 



Those properties are of course supported, but where does one put the classes (or jar containing these classes)? I tried for hours at end and asked in the JBoss forum, but it never became clear. The documentation doesn't mention it either. See this for my question about this: https://developer.jboss.org/thread/254106

Would be really cool if the location could become clear. Thanks!

Kind regards,
Arjan Tijms


















 


Kind regards,
Arjan Tijms


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: SimpleRoleGroup#roles

Philippe Marschall
In reply to this post by Stefan Guilhen
Very well, we can do that. How do you feel about breaking the serialized
form?


On 08.06.2015 15:20, Stefan Guilhen wrote:

> The order of the roles is not important. I think we should change the
> SimpleRole implementation in PicketBox to use Collection (HashSet)
> reference instead of List and then change any WildFly code that might be
> using the List reference.
>
> On 06/08/2015 03:25 AM, Philippe Marschall wrote:
>> Hi
>>
>> I’m aware this may no technically be the right list to discuss this
>> but this list is impacted by this and fairly active.
>>
>> During load testing of our application we found a case we spend 10% of
>> your CPU time in SimpleRole#equals (see attachment). This is because
>> SimpleRoleGroup uses an ArrayList to maintain a unique set of roles.
>> As a result it has to call ArrayList#contains a lot, which is itself
>> O(n). In fact because that’s done when iterating over all the roles it
>> becomes O(n^2). In our case our principals can have up to 200 roles. I
>> don’t know if this is exceptionally many or a common case.
>>
>> We would like to work on a patch but we need your guidance. There are
>> several possible solutions and it all comes down to whether we can
>> change the List in the RoleGroup interface to a Collection. All the
>> users we searched for in WildFly only used the return value for
>> iterating over, nobody used the index. In fact they all used it an a
>> for-each loop so the change would even be source compatible but
>> unfortunately not binary compatible. If we can change the interface
>> then we can just change the ArrayList in SimpleRoleGroup to a HashSet
>> and be done with it. If the order is important the we can either use a
>> TreeSet or a LinkedHashSet.
>> If changing the RoleGroup interface is not possible then there are two
>> other possibilities. The first is only internally using a Set but in
>> getRoles perform a copy. This would produce more garbage. The second
>> option would be a having a Set and List (to avoid having to do copies)
>> in SimpleRoleGroup. This would avoid having to do a copy and the Set
>> can be used for contains checks. Only removeRole would still be O(n)
>> due to the scan over ArrayList. The only user we found was
>> OptionsRoleMappingProvider.
>> All of these would change the serialized form. If it is important to
>> support reading old instances that could be added as well.
>>
>> Cheers
>> Philippe
>>
>>
>> _______________________________________________
>> wildfly-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>
>
>
>
> _______________________________________________
> wildfly-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: SimpleRoleGroup#roles

David Lloyd-2
You don't have to break the serialized form to make this kind of change.
  Have a look at this:
https://docs.oracle.com/javase/7/docs/platform/serialization/spec/serial-arch.html#6250

Set that up, plus a customized readObject()/writeObject(), and you can
retain serialization compatibility.

On 06/09/2015 12:37 AM, Philippe Marschall wrote:

> Very well, we can do that. How do you feel about breaking the serialized
> form?
>
>
> On 08.06.2015 15:20, Stefan Guilhen wrote:
>> The order of the roles is not important. I think we should change the
>> SimpleRole implementation in PicketBox to use Collection (HashSet)
>> reference instead of List and then change any WildFly code that might be
>> using the List reference.
>>
>> On 06/08/2015 03:25 AM, Philippe Marschall wrote:
>>> Hi
>>>
>>> I’m aware this may no technically be the right list to discuss this
>>> but this list is impacted by this and fairly active.
>>>
>>> During load testing of our application we found a case we spend 10% of
>>> your CPU time in SimpleRole#equals (see attachment). This is because
>>> SimpleRoleGroup uses an ArrayList to maintain a unique set of roles.
>>> As a result it has to call ArrayList#contains a lot, which is itself
>>> O(n). In fact because that’s done when iterating over all the roles it
>>> becomes O(n^2). In our case our principals can have up to 200 roles. I
>>> don’t know if this is exceptionally many or a common case.
>>>
>>> We would like to work on a patch but we need your guidance. There are
>>> several possible solutions and it all comes down to whether we can
>>> change the List in the RoleGroup interface to a Collection. All the
>>> users we searched for in WildFly only used the return value for
>>> iterating over, nobody used the index. In fact they all used it an a
>>> for-each loop so the change would even be source compatible but
>>> unfortunately not binary compatible. If we can change the interface
>>> then we can just change the ArrayList in SimpleRoleGroup to a HashSet
>>> and be done with it. If the order is important the we can either use a
>>> TreeSet or a LinkedHashSet.
>>> If changing the RoleGroup interface is not possible then there are two
>>> other possibilities. The first is only internally using a Set but in
>>> getRoles perform a copy. This would produce more garbage. The second
>>> option would be a having a Set and List (to avoid having to do copies)
>>> in SimpleRoleGroup. This would avoid having to do a copy and the Set
>>> can be used for contains checks. Only removeRole would still be O(n)
>>> due to the scan over ArrayList. The only user we found was
>>> OptionsRoleMappingProvider.
>>> All of these would change the serialized form. If it is important to
>>> support reading old instances that could be added as well.
>>>
>>> Cheers
>>> Philippe
>>>
>>>
>>> _______________________________________________
>>> wildfly-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>>
>>
>>
>>
>> _______________________________________________
>> wildfly-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>>
> _______________________________________________
> wildfly-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>

--
- DML
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: SimpleRoleGroup#roles

arjan.tijms
In reply to this post by arjan.tijms
Hi,

On Mon, Jun 8, 2015 at 9:04 PM, arjan tijms <[hidden email]> wrote:
Having said that, the documentation does really seem to be missing a section about custom JACC providers so I went to check the TCK setup. It looks like the TCK JACC providers are bundled in a jar and this jar is being set as a resource of the org.jboss.as.security module. I'm not sure why it was done this way but I believe it should be also possible to define your own module containing the classes and then wire it to the security module as a dependency instead of a resource.

Hmmm, how would one go about doing that exactly? I think I created a module for my custom JACC provider, then set that as a dependency for the security module (since that was the place the default implementation lives), but it again did not work (class not found exceptions). Could well be the case that I did something wrong, so an example would be great.

Sorry to ask again, but still wondering how to set a custom JACC provider ;)

Incidentally, I found a rather grave bug in the default JACC provider where it checks principals purely based on their name, without taking their type into account. See https://issues.jboss.org/browse/WFLY-5740

Kind regards,
Arjan Tijms





_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev