Re: Token creation is not thread safe

2018-04-13 Thread Francesco Chicchiriccò 

On 13/04/2018 09:48, Isuranga Perera wrote:

Hi Francesco,

Yes, that will fix the problem.


Glad we agree :-)
I'll set SYNCOPE-1301; please close the PR #70.

Regards.


On Fri, Apr 13, 2018 at 1:00 PM, Francesco Chicchiriccò 
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
sav

Re: Token creation is not thread safe

2018-04-13 Thread Isuranga Perera 
Hi Francesco,

Yes, that will fix the problem.

Best Regards
Isuranga Perera

On Fri, Apr 13, 2018 at 1:00 PM, Francesco Chicchiriccò  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 th

Re: Token creation is not thread safe

2018-04-13 Thread Francesco Chicchiriccò 

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


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ò 
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 to

Re: Token creation is not thread safe

2018-04-09 Thread Isuranga Perera 
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ò 
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 a

Re: Token creation is not thread safe

2018-04-09 Thread Francesco Chicchiriccò 

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

Re: Token creation is not thread safe

2018-04-09 Thread Isuranga Perera 
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ò 
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:/

Re: Token creation is not thread safe

2018-04-09 Thread Francesco Chicchiriccò 

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ò 
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 > wrote:

Re: Token creation is not thread safe

2018-04-09 Thread Isuranga Perera 
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ò 
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/jav

Re: Token creation is not thread safe

2018-04-09 Thread Francesco Chicchiriccò 

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/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/job/ExpiredAccessTokenCleanup.java

For additional security, we might want to impose a UNIQUE constraint on

https://github.com/apache/syncope/blob/master/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/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ò 
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 > 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/syncope/blob/master/core/provision
ing-java/src/main/java/org/apache/syncope/core/provisioni
ng/java/data/AccessTokenDataBinderImpl.java#L104

Re: Token creation is not thread safe

2018-04-09 Thread Isuranga Perera 
 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.

On Mon, Apr 9, 2018 at 1:12 PM, Francesco Chicchiriccò 
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 > 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/syncope/blob/master/core/provision
 ing-java/src/main/java/org/apache/syncope/core/provisioni
 ng/java/data/AccessTokenDataBinderImpl.java#L104
  

  [2]
  https://github.com/apache/syncope/blob/master/core/provision
 ing-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
>

Re: Token creation is not thread safe

2018-04-09 Thread Francesco Chicchiriccò 

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/persistence-jpa/src/main/resources/domains/MasterDomain.xml#L57-L59

Regards.


On Mon, Apr 9, 2018 at 12:20 PM, Francesco Chicchiriccò  
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 > 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/syncope/blob/master/core/provision
ing-java/src/main/java/org/apache/syncope/core/provisioni
ng/java/data/AccessTokenDataBinderImpl.java#L104
 

 [2]
 https://github.com/apache/syncope/blob/master/core/provision
ing-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/

Re: Token creation is not thread safe

2018-04-09 Thread Isuranga Perera 
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.

Best Regards

On Mon, Apr 9, 2018 at 12:20 PM, Francesco Chicchiriccò  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 > 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/syncope/blob/master/core/provision
>> ing-java/src/main/java/org/apache/syncope/core/provisioni
>> ng/java/data/AccessTokenDataBinderImpl.java#L104
>> > ning-java/src/main/java/org/apache/syncope/core/provisioni
>> ng/java/data/AccessTokenDataBinderImpl.java#L104>
>>
>> [2]
>> https://github.com/apache/syncope/blob/master/core/provision
>> ing-java/src/main/java/org/apache/syncope/core/provisioni
>> ng/java/data/AccessTokenDataBinderImpl.java#L113
>> > 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/
>
>

Re: Token creation is not thread safe

2018-04-08 Thread Francesco Chicchiriccò 

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/AccessTokenServiceImpl.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/AbstractTransactionalLogic.java#L29

Regards.

[1] 
https://github.com/apache/syncope/blob/master/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java#L119


Best Regards
Isuranga Perera

On Mon, Apr 9, 2018 at 11:42 AM, Francesco Chicchiriccò 
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/syncope/blob/master/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java#L104



[2]

https://github.com/apache/syncope/blob/master/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/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/

Re: Token creation is not thread safe

2018-04-08 Thread Isuranga Perera 
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.
[1]
https://github.com/apache/syncope/blob/master/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java#L119

Best Regards
Isuranga Perera

On Mon, Apr 9, 2018 at 11:42 AM, Francesco Chicchiriccò  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/syncope/blob/master/core/provision
>> ing-java/src/main/java/org/apache/syncope/core/provisioni
>> ng/java/data/AccessTokenDataBinderImpl.java#L104
>>
>> [2] https://github.com/apache/syncope/blob/master/core/provision
>> ing-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/
>
>

Re: Token creation is not thread safe

2018-04-08 Thread Francesco Chicchiriccò 

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/syncope/blob/master/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java#L104


[2] 
https://github.com/apache/syncope/blob/master/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/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/

Token creation is not thread safe

2018-04-08 Thread Isuranga Perera 
Hi All,

Token create method in AccessTokenDataBinderImpl[1] is not thread safe.
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/syncope/blob/master/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java#L104

[2]
https://github.com/apache/syncope/blob/master/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java#L113

Best Regards
Isuranga Perera