New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYNCOPE-1301] fixed #70

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@IsurangaPerera
Contributor

IsurangaPerera commented Apr 12, 2018

No description provided.

@@ -115,6 +115,16 @@ public AccessToken save(final AccessToken accessToken) {
return entityManager().merge(accessToken);
}
@Override
@Transactional(rollbackFor = Throwable.class)

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

Please remove this annotation, it is redundant..

if (existing != null) {
entityManager().remove(existing);
}
entityManager().persist(accessToken);

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

replace with save(accessToken) and make this method return AccessToken as save() does.

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

Also, replace entityManager().remove(existing) with delete(existing), thanks.

@@ -34,6 +34,8 @@
AccessToken save(AccessToken accessToken);
void merge(AccessToken accessToken);

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

Change it into

AccessToken merge(AccessToken accessToken);

This comment has been minimized.

@IsurangaPerera

IsurangaPerera Apr 12, 2018

Contributor

Shall I keep the method name same(merge) or change it?

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

maybe replace() makes more sense

if (replaceExisting && existing != null) {
accessTokenDAO.delete(existing);
accessTokenDAO.merge(accessToken);

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

what about the replaceExisting flag? You just removed it!

This comment has been minimized.

@IsurangaPerera

IsurangaPerera Apr 12, 2018

Contributor

yes, merge function simply ensure the uniqueness constraint by checking if the token exists and replace it if so. otherwise, it will create a new register. so no need to perform this check here

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

replaceExisting is an argument of AccesstTokenDataBinderImpl#create that your change would simply ignore.
Not good: it must be checked.

This comment has been minimized.

@IsurangaPerera

IsurangaPerera Apr 12, 2018

Contributor

It is not completely ignored. it is checked by the second if condition as before

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

So, the current implementation will replace the token only if replaceExisting is true; with your change, the token will be replaced anyway, because JPAAccessTokenDAO#merge will do

if (existing != null) {
            entityManager().remove(existing);
        }

while the current code does

        if (replaceExisting && existing != null) {
            accessTokenDAO.delete(existing);
        }

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

To me, it should be enough to pass the replaceExisting flag to the new AccessTokenDAO#replace method, and behave accordingly.

This comment has been minimized.

@IsurangaPerera

IsurangaPerera Apr 12, 2018

Contributor

Actually that way there can be 2 tokens at a given moment. Suppose there exist an access token already. when trying to replace it in the old way it creates and saves another token (at this time there is 2 token which is against the unique constraint).

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

That's why we are not enforcing the unique constraint (hence, NOT changing @Column(nullable = true): the replaceExisting flag exists for a reason, do not alter such behavior as there might be some code relying on it.

This comment has been minimized.

@IsurangaPerera

IsurangaPerera Apr 12, 2018

Contributor

so you're suggesting keep this code segment intact right?
if (replaceExisting && existing != null) {
accessTokenDAO.delete(existing);
}

or remove unique = true from the model?

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

both: remove unique = true (e.g. revert all changes there, as said) AND keep the same code

@@ -44,7 +44,7 @@
@Temporal(TemporalType.TIMESTAMP)
private Date expiryTime;
@Column(nullable = true)
@Column(unique = true)

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

As said during the initial mail exchange, I am not sure of the reason why owner is currently set as nullable, and I don't see enough reasons to change it, please revert.

This comment has been minimized.

@IsurangaPerera

IsurangaPerera Apr 12, 2018

Contributor

I think in JPA it is redundant to use nullable = true this is the default scenario or all columns in JPA. Should I still change it?

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

Just revert, I know nullable = true is the default, but I don't see reasons to change.

delete(existing.getKey());
}
return entityManager().merge(accessToken);

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

replace entityManager().merge(accessToken) with save(accesToken)

@@ -44,7 +44,7 @@
@Temporal(TemporalType.TIMESTAMP)
private Date expiryTime;
@Column(nullable = true)
@Column(nullable = true, unique = true)

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

as said, revert any change here

@@ -34,6 +34,8 @@
AccessToken save(AccessToken accessToken);
AccessToken replace(AccessToken accessToken);

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

This should be

AccessToken replace(boolean replaceExisting, AccessToken accessToken);
@Override
public AccessToken replace(final AccessToken accessToken) {
AccessToken existing = findByOwner(accessToken.getOwner());
if (existing != null) {

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

if (replaceExisting && existing != null) {

delete(existing.getKey());
}
return save(accessToken);

This comment has been minimized.

@ilgrosso

ilgrosso Apr 12, 2018

Member

return replaceExisting ? save(accessToken) : existing;

@IsurangaPerera

This comment has been minimized.

Contributor

IsurangaPerera commented Apr 12, 2018

@ilgrosso without enforcing the UNIQUE constraint on the owner as we discussed in the mail I can't see a valid reason for merge these changes

@ilgrosso

This comment has been minimized.

Member

ilgrosso commented Apr 12, 2018

@IsurangaPerera the point is that the existence of replaceFlag (I did not realize that at first) is what prevents to enforce the UNIQUE constraint.

@IsurangaPerera

This comment has been minimized.

Contributor

IsurangaPerera commented Apr 12, 2018

@ilgrosso Can I know the business rule associated with the access token. Can one user has more than one access token in an any given scenario?

@ilgrosso

This comment has been minimized.

Member

ilgrosso commented Apr 12, 2018

@IsurangaPerera I don't remember the details, but what I can see from the source three is that AccessTokenDataBinderImpl#create is invoked in two places:

  1. https://github.com/apache/syncope/blob/master/core/logic/src/main/java/org/apache/syncope/core/logic/AccessTokenLogic.java#L84 (plain login)
  2. https://github.com/apache/syncope/blob/master/ext/saml2sp/logic/src/main/java/org/apache/syncope/core/logic/SAML2SPLogic.java#L549 (SAML 2.0 login)

The former provides replaceExisting as false, the latter as true.

From the code in AccessTokenDataBinderImpl#create I can see that:

  • for plain login, JWT is generated only at first invocation
  • for SAML 2.0 login, JWT is generated at every invocation, and existing JWT is replaced if existing

Moreover, I cannot recall exactly why the UNIQUE constraint is not imposed to AccessToken's owner.

@ilgrosso

This comment has been minimized.

Member

ilgrosso commented Apr 12, 2018

Ok, let's try this way: please re-introduce the UNIQUE constraint but rework the whole AccessTokenDataBinderImpl#create to comply with the two invocation scenarios as reported above.

Do you think it could be feasible?

@IsurangaPerera

This comment has been minimized.

Contributor

IsurangaPerera commented Apr 12, 2018

@ilgrosso As I understand in SAML SP logic always replaced. So even when we logged, as usual, the access token may changed by SAML SP. So I can understand the importance of what replaceExisitng flag does. After imposing the UNIQUE constraint as in my implementation replaceExisting == true works as expected(always).But sometimes even if the flag is false the token may be replaced (scenario discussed in mail thread). But this is only when the same user tries to log in at the same time & thread not safe problem aise. Anyway this approach is far better than using locks which causes performance drop and this is a rare case as well. What do you think?

@ilgrosso

This comment has been minimized.

Member

ilgrosso commented Apr 12, 2018

@IsurangaPerera the only effect of UNIQUE constraint will be that a validation error is raised whenever a second AccessToken is created with same owner of an existing one.
Are you simply proposing to add such constraint and leave the rest of the code as is?

@IsurangaPerera

This comment has been minimized.

Contributor

IsurangaPerera commented Apr 12, 2018

@ilgrosso A validation error will be raised in the worst case(As I see the thread safe problem won't just affect the login as we discussed Saml as well. So Unique constraint will only prevent those scenarios. the advantage is the validation error is the worst case. It won't negatively affect the flow of the system like in thrad safe problem(Users being blocked from login)

Please correct me If I'm wrong 🙂

@ilgrosso

This comment has been minimized.

Member

ilgrosso commented Apr 12, 2018

Ok so...

Are you simply proposing to add such constraint and leave the rest of the code as is?

e.g. to change again this PR by reverting all changes and adding the UNIQUE constraint?

@IsurangaPerera

This comment has been minimized.

Contributor

IsurangaPerera commented Apr 12, 2018

Yes, What if we revert it to the initial state?

@ilgrosso

This comment has been minimized.

Member

ilgrosso commented Apr 12, 2018

I would stick with minimal changes, so this PR should only change JPAAccessToken.

@IsurangaPerera

This comment has been minimized.

Contributor

IsurangaPerera commented Apr 12, 2018

That way replace won't work since it saves first (2 tokens exist.violate UNIQUE constraint) and deletes next. That way it will only delete the existing one which results in no token at all.

@ilgrosso

This comment has been minimized.

Member

ilgrosso commented Apr 12, 2018

That way replace won't work since it saves first (2 tokens exist.violate UNIQUE constraint) and deletes next. That way it will only delete the existing one which results in no token at all.

You are right, but as you can see, in the current implementation save and delete do not happen under the same conditions.

@IsurangaPerera

This comment has been minimized.

Contributor

IsurangaPerera commented Apr 12, 2018

@ilgrosso Yes, that's why we have to see it from a different perspective.
The initial state doesn't change the logic of replaceExisting == true
But it partially affects replaceExisting == False (This affects only if the thread safe problems occur). This means when the issue syncope-1301 occurs instead of creating a new token for the same user this will replace the existing one (rare situation)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment