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/ > >
