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