Since such condition can happen only if the same user tries to login from 2 mediums at the same, this is rarely happen. However that slight chance may prevent that particular user from login to the system until all or all-1 access tokens are expired. Using the UNIQUE constraint will definitely will provide a better security and furthermore will make the model more meaningful. On the other hand this will break the token replacing functionality since it first create a token (at this time there are 2 tokens in the db) and delete the last one.
What I propose is writing a separate query to replace tokens instead of using save & delete queries separately and furthermore we can use a new query to save tokens without affecting the UNIQUE constraints so that no need to mess with threading & @Transactional properties. On Mon, Apr 9, 2018 at 2:06 PM, Francesco Chicchiriccò <ilgro...@apache.org> wrote: > 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/provision > ing-java/src/main/java/org/apache/syncope/core/provisioni > ng/java/job/ExpiredAccessTokenCleanup.java > > For additional security, we might want to impose a UNIQUE constraint on > > https://github.com/apache/syncope/blob/master/core/persisten > ce-jpa/src/main/java/org/apache/syncope/core/persistenc > e/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ò < >> ilgro...@apache.org> >> 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ò < >>> >>>> ilgro...@apache.org> 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ò < >>>>>> ilgro...@apache.org <mailto:ilgro...@apache.org>> 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/sync >>>>>> ope/blob/master/core/provision >>>>>> ing-java/src/main/java/org/apache/syncope/core/provisioni >>>>>> ng/java/data/AccessTokenDataBinderImpl.java#L104 >>>>>> <https://github.com/apache/syn >>>>>> cope/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/sync >>>>>> ope/blob/master/core/provision >>>>>> ing-java/src/main/java/org/apache/syncope/core/provisioni >>>>>> ng/java/data/AccessTokenDataBinderImpl.java#L113 >>>>>> <https://github.com/apache/syn >>>>>> cope/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/ > >
