On 09/04/2018 10:14, Isuranga Perera wrote:
The Read Committed isolation level ensures that data can only be read by a
transaction if it is in the committed state. It doesn't completely isolate
this transaction(create) hence some other transaction can still use this
method which results in the behavior I explained previously. Ideally If
we're gonna use @Transactional annotation the isolation level should be
serialized for create operation. Please correct me If I'm wrong.
I see your point - while I don't completely agree about the likelihood
of such race condition to actually happen.
At worse, you might end up in having two distinct JWT (Access Tokens)
values for a single user, which will anyway be subject to expiration due to
https://github.com/apache/syncope/blob/master/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/job/ExpiredAccessTokenCleanup.java
For additional security, we might want to impose a UNIQUE constraint on
https://github.com/apache/syncope/blob/master/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/JPAAccessToken.java#L48
(not sure to remember why the column is currently set as nullable, though).
With UNIQUE owner, the step (5) in your sequence below will fail anyway.
Again, given the chances that the race condition applies, and
considering what would be the harm (nearly none, to me), I would rather
avoid any modification rather than imposing the UNIQUE constraint.
Regards.
On Mon, Apr 9, 2018 at 1:12 PM, Francesco Chicchiriccò <[email protected]>
wrote:
On 09/04/2018 09:30, Isuranga Perera wrote:
Hi Francesco,
Yes there is @Transactional annotation. But it haven't set the isolation
property as well as the propagation property. Based on the default values
set this thread safe problem will still occur. Please correct me if I'm
wrong.
The transaction isolation level is set in
https://github.com/apache/syncope/blob/master/core/persisten
ce-jpa/src/main/resources/domains/MasterDomain.xml#L57-L59
Regards.
On Mon, Apr 9, 2018 at 12:20 PM, Francesco Chicchiriccò <
[email protected]> wrote:
On 09/04/2018 08:46, Isuranga Perera wrote:
Hi Francesco,
I will take a scenario. Suppose a scenario where thread A & thread B try
to login user admin.
1. thread A checks if a token exist for the user admin (suppose
currently there is no token associated with the admin)
2. Then thread A execute following logic[1] to create and save the
token.
3. Before thread A save the token for user admin thread B checks if a
token exist for user admin (since the toked created by thread A is
not yet saved *exist == null*)
4. Then thread A complete the creation of token (and saving)
5. Thread B also complete the creation and saving of the token.
That way there can be 2 tokens for a particular user.
You analysis does not take into any account the fact of the constraints
imposed by the @Service annotation in
https://github.com/apache/syncope/blob/master/core/rest-cxf/
src/main/java/org/apache/syncope/core/rest/cxf/service/Acces
sTokenServiceImpl.java#L35
(e.g. the place when external requests can come in) nor by the
@Transactional annotation injected into
https://github.com/apache/syncope/blob/master/core/logic/
src/main/java/org/apache/syncope/core/logic/AccessTokenLogic.java#L80
via
https://github.com/apache/syncope/blob/master/core/logic/
src/main/java/org/apache/syncope/core/logic/AbstractTra
nsactionalLogic.java#L29
Regards.
[1] https://github.com/apache/syncope/blob/master/core/provision
ing-java/src/main/java/org/apache/syncope/core/provisioni
ng/java/data/AccessTokenDataBinderImpl.java#L119
Best Regards
Isuranga Perera
On Mon, Apr 9, 2018 at 11:42 AM, Francesco Chicchiriccò <
[email protected] <mailto:[email protected]>> wrote:
On 09/04/2018 07:07, Isuranga Perera wrote:
Hi All,
Token create method in AccessTokenDataBinderImpl[1] is not
thread safe.
Could you please explain why you're affirming this?
This could result in several problems including
* Exist 2 different access token for a particular user at a
given
time which may result in an exception thrown by method
call[2]
since it expects a single token a given user.
In addition to that token replace is implemented as a
combination of 2 different functionalities. Since the method
is not thread safe this may cause some unexpected behaviors
(since there can be 2 tokens exist for a particular user. same
scenario as above).
Appreciate your insight on the $subject.
[1]
https://github.com/apache/syncope/blob/master/core/provision
ing-java/src/main/java/org/apache/syncope/core/provisioni
ng/java/data/AccessTokenDataBinderImpl.java#L104
<https://github.com/apache/syncope/blob/master/core/provisio
ning-java/src/main/java/org/apache/syncope/core/provisioni
ng/java/data/AccessTokenDataBinderImpl.java#L104>
[2]
https://github.com/apache/syncope/blob/master/core/provision
ing-java/src/main/java/org/apache/syncope/core/provisioni
ng/java/data/AccessTokenDataBinderImpl.java#L113
<https://github.com/apache/syncope/blob/master/core/provisio
ning-java/src/main/java/org/apache/syncope/core/provisioni
ng/java/data/AccessTokenDataBinderImpl.java#L113>
Best Regards
Isuranga Perera
--
Francesco Chicchiriccò
Tirasa - Open Source Excellence
http://www.tirasa.net/
Member at The Apache Software Foundation
Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
http://home.apache.org/~ilgrosso/
