Sure will work on that. Shall I create a JIRA? Sorry for the delay will submit the ICLA asap
Best Regards Isuranga Perera On Mon, Apr 9, 2018 at 2:45 PM, Francesco Chicchiriccò <ilgro...@apache.org> wrote: > On 09/04/2018 11:10, Isuranga Perera wrote: > >> 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. >> > > If you can come up with a proposal which works with all the supported > DBMSes, then please go on. > > As already asked as comment in your recent PR: did you submit an ICLA for > your contributions? Thanks. > > Regards. > > 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/ > >
