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

Allow use of non-numeric (e.g. UUID) values for ObjectIdentity.getIdentifier() #4424

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@pwheel
Contributor

pwheel commented Jun 28, 2017

This PR fixes #1224.

I discussed the general idea for this change over a year ago with @rwinch on gitter and shared pwheel@f388281#comments with him.

This took a while to get ready for a PR as when I got round to writing the tests properly, there was a problem with how HSQLDB was handling converting UUID to varchar so I had to submit a fix for that which came out in HSQLDB 2.4.0 earlier this year.

The change allows for the fully qualified class name of the ObjectIdentity identifier type to be specified in the acl_class table and a ConversionService can be provided to BasicLookupStrategy to convert from String to the actual identifier type.

There are the following other changes:

  1. BasicLookupStrategy has a new property, aclClassIdSupported, which
    is used to retrieve the new column from the database. This preserves
    backwards-compatibility, as it is false by default.

  2. JdbcMutableAclService has the same property, aclClassIdSupported,
    which is needed to modify the insert statement to write to the
    new column. Defaults to false for backwards-compatibility.

  3. Tests have been updated to verify both the existing functionality
    for backwards-compatibility and the new functionality.

Other things to call out:

  1. I haven't updated the documentation; I can do that but wanted feedback first as it may result in changes.

  2. I've changed the default schemas to use varchar for acl_object_identity.object_id_identity column, but I haven't added the new acl_class.class_id_type column by default. That seems a bit inconsistent in retrospect but again appreciate your feedback here.

  3. Master isn't building atm (fails with sample.HelloWebfluxApplicationTests on my machine at least) but the tests for the acl module pass. Apologies for submitting in this state, I wanted to kick-start the process.

Cheers,
Paul

Allow use of non-numeric (e.g. UUID) values for ObjectIdentity.getIde…
…ntifier()

Prior to this commit, the ObjectIdentity id had to be a number. This
commit allows for domain objects to use UUIDs as their identifier. The
fully qualified class name of the identifier type can be specified
in the acl_object_identity table and a ConversionService can be provided
to BasicLookupStrategy to convert from String to the actual identifier
type.

There are the following other changes:

 - BasicLookupStrategy has a new property, aclClassIdSupported, which
 is used to retrieve the new column from the database. This preserves
 backwards-compatibility, as it is false by default.

 - JdbcMutableAclService has the same property, aclClassIdSupported,
 which is needed to modify the insert statement to write to the
 new column. Defaults to false for backwards-compatibility.

 - Tests have been updated to verify both the existing functionality
 for backwards-compatibility and the new functionality.

Fixes SEC-972
@graeme-dougal

This comment has been minimized.

Show comment
Hide comment
@graeme-dougal

graeme-dougal Jul 14, 2017

Any idea if / when this will be implemented

graeme-dougal commented Jul 14, 2017

Any idea if / when this will be implemented

@pwheel

This comment has been minimized.

Show comment
Hide comment
@pwheel

pwheel Jul 21, 2017

Contributor

Merged changes from master that fixed the broken tests, all tests pass locally now

Contributor

pwheel commented Jul 21, 2017

Merged changes from master that fixed the broken tests, all tests pass locally now

@Andy2003

This comment has been minimized.

Show comment
Hide comment
@Andy2003

Andy2003 Aug 9, 2017

I tested this PR and it works for me

Andy2003 commented Aug 9, 2017

I tested this PR and it works for me

@sterowney

This comment has been minimized.

Show comment
Hide comment
@sterowney

sterowney commented Oct 3, 2017

👍

@truongvanluan

This comment has been minimized.

Show comment
Hide comment
@truongvanluan

truongvanluan Oct 11, 2017

Hello @pwheel , there is still one class needs to be modified, it is JdbcAclService. If you look into its findChildren method (which is used by JdbcMutableAclService), you should see this
String javaType = rs.getString("class");
Long identifier = new Long(rs.getLong("obj_id"));

And this is the obj_id per DEFAULT_SELECT_ACL_WITH_PARENT_SQL
select obj.object_id_identity as obj_id

So because we are using Uuid this cast will cause a problem.
Can we have this fixed as well ? (or correct me if this is not an issue). Thanks anyway for this PR, really appreciate it!

truongvanluan commented Oct 11, 2017

Hello @pwheel , there is still one class needs to be modified, it is JdbcAclService. If you look into its findChildren method (which is used by JdbcMutableAclService), you should see this
String javaType = rs.getString("class");
Long identifier = new Long(rs.getLong("obj_id"));

And this is the obj_id per DEFAULT_SELECT_ACL_WITH_PARENT_SQL
select obj.object_id_identity as obj_id

So because we are using Uuid this cast will cause a problem.
Can we have this fixed as well ? (or correct me if this is not an issue). Thanks anyway for this PR, really appreciate it!

pwheel added some commits Oct 21, 2017

SEC-972
Feedback from PR, the JdbcAclService.findChildren method would have been broken.
As logic now needs to be in JdbcAclService & BasicLookupStrategy, I've refactored the logic into an AclClassIdUtils class.
Also realised that I'd missed this as JdbcMutableAclServiceTestsWithAclClassId was not covering all scenarios; I needed to create UUID versions of the ObjectIdentity instances to trigger the new behaviour.
@pwheel

This comment has been minimized.

Show comment
Hide comment
@pwheel

pwheel Oct 22, 2017

Contributor

Thanks @truongvanluan good catch. I updated some of the tests to cover this & confirm was a problem. I've done a little bit of refactoring as needed to use some of the logic in to classes now, so created a utility class AclClassIdUtils to handle it.

I had to merge in master as had compile time errors due to Spring 5 snapshot dependencies, & had to do some updates for checkstyle & Apache licence too.

Commit history isn't so clean now… I can squash / rebase again but wanted to push this up for feedback first. Hope that's alright!

Contributor

pwheel commented Oct 22, 2017

Thanks @truongvanluan good catch. I updated some of the tests to cover this & confirm was a problem. I've done a little bit of refactoring as needed to use some of the logic in to classes now, so created a utility class AclClassIdUtils to handle it.

I had to merge in master as had compile time errors due to Spring 5 snapshot dependencies, & had to do some updates for checkstyle & Apache licence too.

Commit history isn't so clean now… I can squash / rebase again but wanted to push this up for feedback first. Hope that's alright!

@rwinch rwinch self-assigned this Oct 30, 2017

@rwinch rwinch added this to the 5.0.0.RC1 milestone Oct 30, 2017

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Oct 30, 2017

Member

Thanks for the PR @pwheel! This is merged into master via 6decf1c

Member

rwinch commented Oct 30, 2017

Thanks for the PR @pwheel! This is merged into master via 6decf1c

@rwinch rwinch closed this Oct 30, 2017

@pwheel

This comment has been minimized.

Show comment
Hide comment
@pwheel

pwheel Nov 3, 2017

Contributor

That's awesome, thanks @rwinch!

Contributor

pwheel commented Nov 3, 2017

That's awesome, thanks @rwinch!

@graeme-dougal

This comment has been minimized.

Show comment
Hide comment
@graeme-dougal

graeme-dougal Nov 8, 2017

Hi thanks for this, been waiting a while for this to be implemented so been keen to try it out...

I've added setAclClassIdSupported(true) to both my LookupStrategy and JdbcMutableAclService

However, I'm still getting an error in AclClassIdUtils as it fails because there is no ConversionService set so it therefore attempts to convert to Long when I have a String

The offending method in AclClassIdUtils is
private <T> boolean canConvertFromStringTo(Class<T> targetType) { return hasConversionService() && conversionService.canConvert(String.class, targetType); }
So my questions is - do you need to set the conversion service and if so how ? Otherwise, is there something else I am missing ?

Thanks
Graeme

graeme-dougal commented Nov 8, 2017

Hi thanks for this, been waiting a while for this to be implemented so been keen to try it out...

I've added setAclClassIdSupported(true) to both my LookupStrategy and JdbcMutableAclService

However, I'm still getting an error in AclClassIdUtils as it fails because there is no ConversionService set so it therefore attempts to convert to Long when I have a String

The offending method in AclClassIdUtils is
private <T> boolean canConvertFromStringTo(Class<T> targetType) { return hasConversionService() && conversionService.canConvert(String.class, targetType); }
So my questions is - do you need to set the conversion service and if so how ? Otherwise, is there something else I am missing ?

Thanks
Graeme

@pwheel

This comment has been minimized.

Show comment
Hide comment
@pwheel

pwheel Nov 11, 2017

Contributor

Hi @graeme-dougal, yes you do. If you take a look at the test setup here that should hopefully answer your question -

Cheers,
Paul

Contributor

pwheel commented Nov 11, 2017

Hi @graeme-dougal, yes you do. If you take a look at the test setup here that should hopefully answer your question -

Cheers,
Paul

@graeme-dougal

This comment has been minimized.

Show comment
Hide comment
@graeme-dougal

graeme-dougal Nov 12, 2017

Hi Paul

Thanks for response. I'm using java config rather than xml config. My issue with wiring in AclClassIdUtils is that the class is not public so cannot be accessed.

I'd like to do the following but this won't compile due to the class not being public.
`
@bean
public AclClassIdUtils aclClassIdUtils() {

    AclClassIdUtils classIdUtils = new AclClassIdUtils();
    classIdUtils.setConversionService(new DefaultConversionService());
    
    return classIdUtils
}

`

Thanks
Graeme

graeme-dougal commented Nov 12, 2017

Hi Paul

Thanks for response. I'm using java config rather than xml config. My issue with wiring in AclClassIdUtils is that the class is not public so cannot be accessed.

I'd like to do the following but this won't compile due to the class not being public.
`
@bean
public AclClassIdUtils aclClassIdUtils() {

    AclClassIdUtils classIdUtils = new AclClassIdUtils();
    classIdUtils.setConversionService(new DefaultConversionService());
    
    return classIdUtils
}

`

Thanks
Graeme

@pwheel

This comment has been minimized.

Show comment
Hide comment
@pwheel

pwheel Nov 13, 2017

Contributor

Ah, yeah, got it, doh. I refactored this a couple of times and originally thought this class would be internal.

It should be public now, for sure. Raised #4814 will fix this week

Contributor

pwheel commented Nov 13, 2017

Ah, yeah, got it, doh. I refactored this a couple of times and originally thought this class would be internal.

It should be public now, for sure. Raised #4814 will fix this week

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