Skip to content
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.

Sign up for GitHub

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

XWIKI-22683: Move org.xwiki.security.authorization.DefaultAuthorizationManager to internal package #3791

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

XWIKI-22683: Move org.xwiki.security.authorization.DefaultAuthorizationManager to internal package #3791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions xwiki-platform-core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,21 @@
</item>
</differences>
</revapi.differences>
<revapi.differences>
<justification>
This class should have never been exposed as an API as it's an internal component implementation.
It has been moved to an internal package following the vote performed on
https://forum.xwiki.org/t/move-defaultauthorizationmanager-to-an-internal-package/15926.
</justification>
<criticality>highlight</criticality>
<differences>
<item>
<ignore>true</ignore>
<code>java.class.removed</code>
<old>class org.xwiki.security.authorization.DefaultAuthorizationManager</old>
</item>
</differences>
</revapi.differences>
</analysisConfiguration>
</configuration>
</plugin>
Expand Down
2 changes: 1 addition & 1 deletion ...form-display-macro/src/test/java/org/xwiki/rendering/internal/macro/DisplayMacroTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
import org.xwiki.rendering.wiki.WikiModel;
import org.xwiki.security.authorization.AuthorizationManager;
import org.xwiki.security.authorization.ContextualAuthorizationManager;
import org.xwiki.security.authorization.DefaultAuthorizationManager;
import org.xwiki.security.internal.authorization.DefaultAuthorizationManager;
import org.xwiki.security.authorization.DocumentAuthorizationManager;
import org.xwiki.security.authorization.Right;
import org.xwiki.test.annotation.AllComponents;
Expand Down
2 changes: 1 addition & 1 deletion ...ro-include/src/test/java/org/xwiki/rendering/internal/macro/include/IncludeMacroTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@
import org.xwiki.rendering.wiki.WikiModel;
import org.xwiki.security.authorization.AuthorExecutor;
import org.xwiki.security.authorization.ContextualAuthorizationManager;
import org.xwiki.security.authorization.DefaultAuthorizationManager;
import org.xwiki.security.internal.authorization.DefaultAuthorizationManager;
import org.xwiki.security.authorization.DocumentAuthorizationManager;
import org.xwiki.security.authorization.Right;
import org.xwiki.test.annotation.AllComponents;
Expand Down
4 changes: 2 additions & 2 deletions ...horization-api/src/main/java/org/xwiki/security/authorization/AuthorizationException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@
public class AuthorizationException extends Exception
{
/** Constant value displayed for null entityReference. */
static final String NULL_ENTITY = "Main Wiki";
public static final String NULL_ENTITY = "Main Wiki";

/** Constant value displayed for null userReference. */
static final String NULL_USER = "Public";
public static final String NULL_USER = "Public";

/** Serialization identifier. */
private static final long serialVersionUID = 1L;
Expand Down
6 changes: 3 additions & 3 deletions ...form-security-authorization-api/src/main/java/org/xwiki/security/authorization/Right.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
this(description.getName(), description.getDefaultState(), description.getTieResolutionPolicy(),
description.getInheritanceOverridePolicy(),
Expand Down Expand Up @@ -420,7 +420,7 @@ public static List<Right> getStandardRights()
*
* @since 13.5RC1
*/
void unregister()
public void unregister()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
Set<EntityType> entityTypes = this.getTargetedEntityType();
synchronized (VALUES) {
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
return new EqualsBuilder()
.append(this.isReadOnly(), description.isReadOnly())
Expand Down
12 changes: 11 additions & 1 deletion ...rization/DefaultAuthorizationManager.java → ...rization/DefaultAuthorizationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
*/
package org.xwiki.security.authorization;
package org.xwiki.security.internal.authorization;

import java.util.Arrays;
import java.util.HashSet;
Expand All @@ -37,6 +37,16 @@
import org.xwiki.security.SecurityReference;
import org.xwiki.security.SecurityReferenceFactory;
import org.xwiki.security.UserSecurityReference;
import org.xwiki.security.authorization.AccessDeniedException;
import org.xwiki.security.authorization.AuthorizationException;
import org.xwiki.security.authorization.AuthorizationManager;
import org.xwiki.security.authorization.Right;
import org.xwiki.security.authorization.RightDescription;
import org.xwiki.security.authorization.RuleState;
import org.xwiki.security.authorization.SecurityAccess;
import org.xwiki.security.authorization.SecurityAccessEntry;
import org.xwiki.security.authorization.SecurityRuleEntry;
import org.xwiki.security.authorization.UnableToRegisterRightException;
import org.xwiki.security.authorization.cache.SecurityCache;
import org.xwiki.security.authorization.cache.SecurityCacheLoader;
import org.xwiki.security.authorization.internal.DocumentRequiredRightsChecker;
Expand Down
2 changes: 1 addition & 1 deletion ...tion/xwiki-platform-security-authorization-api/src/main/resources/META-INF/components.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ org.xwiki.security.authorization.cache.internal.DefaultSecurityCache
org.xwiki.security.authorization.cache.internal.DefaultSecurityCacheLoader
org.xwiki.security.authorization.internal.AuthorizationSettlerProvider
org.xwiki.security.authorization.internal.DocumentRequiredRightsChecker
org.xwiki.security.authorization.DefaultAuthorizationManager
org.xwiki.security.internal.authorization.DefaultAuthorizationManager
org.xwiki.security.authorization.internal.DefaultAuthorizationManagerConfiguration
org.xwiki.security.authorization.internal.DefaultAuthorizationSettler
org.xwiki.security.authorization.internal.PrioritizingAuthorizationSettler
1 change: 1 addition & 0 deletions ...est/java/org/xwiki/security/authorization/DefaultAuthorizationManagerIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
import org.xwiki.security.authorization.testwikis.TestWiki;
import org.xwiki.security.internal.UserBridge;
import org.xwiki.security.internal.XWikiBridge;
import org.xwiki.security.internal.authorization.DefaultAuthorizationManager;
import org.xwiki.test.LogLevel;
import org.xwiki.test.annotation.BeforeComponent;
import org.xwiki.test.annotation.ComponentList;
Expand Down
2 changes: 1 addition & 1 deletion .../test/java/org/xwiki/security/authorization/internal/DefaultAuthorizationSettlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
import org.xwiki.security.SecurityReference;
import org.xwiki.security.UserSecurityReference;
import org.xwiki.security.authorization.AbstractAdditionalRightsTestCase;
import org.xwiki.security.authorization.DefaultAuthorizationManager;
import org.xwiki.security.internal.authorization.DefaultAuthorizationManager;
import org.xwiki.security.authorization.Right;
import org.xwiki.security.authorization.RuleState;
import org.xwiki.security.authorization.SecurityAccess;
Expand Down
2 changes: 1 addition & 1 deletion ...e/src/main/java/org/xwiki/security/authorization/internal/BridgeAuthorizationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import org.xwiki.model.reference.EntityReference;
import org.xwiki.rendering.async.AsyncContext;
import org.xwiki.security.authorization.AccessDeniedException;
import org.xwiki.security.authorization.DefaultAuthorizationManager;
import org.xwiki.security.internal.authorization.DefaultAuthorizationManager;
import org.xwiki.security.authorization.Right;

/**
Expand Down