Add ObjectIdentityGenerator customization to JdbcAclService #10081
Conversation
|
@Ditscheridou Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
spring-projects-issues
added
the
status: waiting-for-triage
|
@Ditscheridou Thank you for signing the Contributor License Agreement! |
There was a problem hiding this comment.
Thanks for the PR, @Ditscheridou! I've left some feedback inline.
Also, in preparation will you please review the contribution guidelines about squashing and formatting commits?
| @@ -152,8 +137,9 @@ public BasicLookupStrategy(DataSource dataSource, AclCache aclCache, | |||
| * @param aclAuthorizationStrategy authorization strategy (required) | |||
| * @param grantingStrategy the PermissionGrantingStrategy | |||
| */ | |||
| public BasicLookupStrategy(DataSource dataSource, AclCache aclCache, | |||
| AclAuthorizationStrategy aclAuthorizationStrategy, PermissionGrantingStrategy grantingStrategy) { | |||
| public BasicLookupStrategy(DataSource dataSource, AclCache aclCache, | |||
There was a problem hiding this comment.
Adding a constructor parameter breaks backward compatibility, so I would recommend that this be moved to a setter.
Additionally, it's quite common in Spring Security for required members to be constructor parameters and optional members to have setters.
There was a problem hiding this comment.
This setter should also confirm that the provided strategy is not null.
There was a problem hiding this comment.
Hi, @Ditscheridou, just one more item that I didn't notice until just now.
| import org.springframework.security.acls.model.PermissionGrantingStrategy; | ||
| import org.springframework.security.acls.model.Sid; | ||
| import org.springframework.security.acls.model.UnloadedSidException; | ||
| import org.springframework.security.acls.domain.*; |
There was a problem hiding this comment.
My apologies, I didn't notice this earlier. Let's please leave package imports exploded. Ideally, import ordering should not change in a PR.
There was a problem hiding this comment.
sorry for that, that happens when you press strg+ shift + o too much. Will fix this!
…inside the BasicLookupStrategy,JdbcAclService There was a problem with hard coded object identity creation inside the BasicLookupStrategy and the JdbcAclService. It was overkill to overwrite these classes only for changing this, so introducing an ObjectIdentityGenerator seems the be the better solution here. At default, the standard ObjectIdentityRetrievalStrategyImpl is used, but can be customized due to setters. Closes spring-projectsgh-10081
|
I reworked my pr to meet your requirements, hopefully i did everything right! |
|
Thanks, @Ditscheridou! I think there is a bit more formatting to take care of. Will you take a look at the diff and ensure that the only changes to the code are the ones needed for your PR? I'm still seeing import changes, and some whitespace changes appear to have been introduced. Along those same lines, please run the Once things are ready again, will you please also shorten the commit title, this simplifies looking through the git log down the road. |
|
Hi, @Ditscheridou! Are you still able to do the requested changes? I'm happy to lend a hand as well. |
|
@jzheaux, sorry for the delay. Im currently busy with the release of our company project, so i will sadly not be able to fix within the next two weeks. Sorry for that, that was not my intention when i openend this pr. But if this can wait, i will come back to this when we rollout this. |
|
Thanks, @Ditscheridou, I think waiting is fine. |
|
Hi, @Ditscheridou, are you able to make the requested changes now? |
sjohnr
added
status: duplicate
sjohnr
changed the title
|
Thanks @Ditscheridou, this is now in I've added a polish commit for the remaining items: 74e3abc |
Basically there should no hard coded creation of an object identity in the implementations, so everywhere where this happens i changed this to a call against the ObjectIdentityGenerator.createObjectIdentity(). So now you can define, easily how this object identity is created.