Hi Francesco, Yes, that will fix the problem.
Best Regards Isuranga Perera On Fri, Apr 13, 2018 at 1:00 PM, Francesco Chicchiriccò <ilgro...@apache.org > wrote: > Hi, > after our discussion on PR #70 [1] yesterday, I took the chance to review > the AccessToken creation logic and committed a change [2] which should fix > your warnings from SYNCOPE-1301. > > Please, take a look at let me know if we can consider SYNCOPE-1301 as > resolved. > > Regards. > > [1] https://github.com/apache/syncope/pull/70 > [2] https://github.com/apache/syncope/commit/24f789932141ee05fa1 > 2d81eca9d43178953f508 > > > On 09/04/2018 13:19, Isuranga Perera wrote: > >> Sure will work on that. I'll give priority to this feature and will >> continue to work on the eclipse project upon the completion of this one. >> >> Best Regards >> Isuranga Perera >> >> On Mon, Apr 9, 2018 at 4:27 PM, Francesco Chicchiriccò < >> ilgro...@apache.org> >> wrote: >> >> On 09/04/2018 11:24, Isuranga Perera wrote: >>> >>> Sure will work on that. Shall I create a JIRA? >>>> >>>> Yes, please. >>> Do set both 2.0.9 and 2.1.0 as fix-for-versions since I will apply your >>> PR >>> both to branches master and 2_0_X. >>> >>> Sorry for the delay will submit the ICLA asap >>> Ok, thanks. >>> >>> >>> Regards. >>> >>> 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/ > >
