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

Reply via email to