XWIKI-22683: Move org.xwiki.security.authorization.DefaultAuthorizationManager to internal package #3791
Conversation
…onManager to internal package
* Move DefaultAuthorizationManager to internal package
* Expose some APIs that were package protected to make them public so
that the component implementation can access them
* Add a revapi ignore to explain the removed public API
| @@ -420,7 +420,7 @@ public static List<Right> getStandardRights() | |||
| * | |||
| * @since 13.5RC1 | |||
| */ | |||
| void unregister() | |||
| public void unregister() | |||
There was a problem hiding this comment.
Shouldn't this be marked as @Unstable as this will be a new public API? Also, considering https://jira.xwiki.org/browse/XWIKI-21012, I'm wondering if it is really a good idea to expose this as a new public API.
Further, from what I can see, this would be the only method on Right that is dangerous, meaning that with this change, it wouldn't be safe anymore to expose a Right object in a script API. I can't find any place where Right is exposed in a script API, however, I actually wanted to do exactly this (expose Right objects in script APIs) for required rights.
There was a problem hiding this comment.
Actually, I just found out that it is already possible to access Right instances with just script right. Making this method public means that any user with script right can unregister any right. I don't think this is a good idea.
There was a problem hiding this comment.
Thanks for checking it, I agree it's probably not a good idea. But I'm not yet sure how to avoid this, creating a SecuredRight wrapper doesn't feel like the right way here.
| @@ -206,7 +206,7 @@ public class Right implements RightDescription, Serializable, Comparable<Right> | |||
| * @param impliedByRights the already existing rights that imply this new right. | |||
| * @since 12.6 | |||
| */ | |||
| Right(RightDescription description, Set<Right> impliedByRights) | |||
| public Right(RightDescription description, Set<Right> impliedByRights) | |||
There was a problem hiding this comment.
This change is in contradiction to the documentation of the method. Either, this method must be kept package-private or the documentation must be changed. To me, making this constructor public seems like a very important API change.
There was a problem hiding this comment.
Interesting, I'm starting to wonder if the component implem wasn't public just because of this... I'm not a big fan of making this public but I'm not sure how to avoid it.
| @@ -535,7 +535,7 @@ public int compareTo(Right other) | |||
| * @param description a right description to compare this right to. | |||
| * @return true if the right is equivalent to the provided description. | |||
| */ | |||
| boolean like(RightDescription description) | |||
| public boolean like(RightDescription description) | |||
There was a problem hiding this comment.
As for the other methods, I think this method should be marked as @Unstable as it is a new part of the public API. I don't see any problems with exposing this method, but I think it's documentation should be made clearer - why does this method exist and when should it be used as opposed to equals() or compareTo.
Jira URL
Changes
https://jira.xwiki.org/browse/XWIKI-22683
Description
Clarifications
Screenshots & Video
Executed Tests
Ran
mvn clean install -Pqualityon modulexwiki-platform-securityExpected merging strategy