org.xwiki.platform:xwiki-platform-oldcore Improper Authorization check for inactive users

@@ -31,9 +31,15 @@
         #largeUserAvatar($xcontext.user)
       </a>
       <div class="brand-links">
+        #define ($logoutLink)
+          <a href="$xwiki.getURL('XWiki.XWikiLogout', 'logout', "xredirect=$escapetool.url($xwiki.relativeRequestURL)")" id="tmLogout" rel="nofollow">$services.icon.renderHTML('log-out') $escapetool.xml($services.localization.render('logout'))</a>
+        #end
         #if ($xcontext.user != 'XWiki.XWikiGuest')
           <a href="$xwiki.getURL($xcontext.user, 'view')" class="brand-user" id="tmUser">$!xwiki.getUserName($xcontext.user, false)</a>
-          <a href="$xwiki.getURL('XWiki.XWikiLogout', 'logout', "xredirect=$escapetool.url($xwiki.relativeRequestURL)")" id="tmLogout" rel="nofollow">$services.icon.renderHTML('log-out') $escapetool.xml($services.localization.render('logout'))</a>
+          $logoutLink
+        #elseif($xcontext.user == 'XWiki.XWikiGuest' && $xcontext.inactiveUserReference)
+          <a href="$xwiki.getURL($xcontext.inactiveUserReference, 'view')" class="brand-user" id="tmUser">$!xwiki.getUserName($xcontext.inactiveUserReference, false)</a>
+          $logoutLink
         #else
           <a href="$xwiki.getURL('XWiki.XWikiLogin', 'login', "xredirect=$escapetool.url($xwiki.relativeRequestURL)&loginLink=1")" id="tmLogin" rel="nofollow">$services.icon.renderHTML('log-in') $escapetool.xml($services.localization.render('login'))</a>
           #if ($xwiki.hasAccessLevel('register', 'XWiki.XWikiPreferences'))

@@ -185,7 +185,7 @@ public Response getNotificationsCount(String useUserPreferences, String userId,
     {
         // Build the response
         Response.ResponseBuilder response;
-        XWikiUser xWikiUser = getXWikiContext().getWiki().getAuthService().checkAuth(getXWikiContext());
+        XWikiUser xWikiUser = getXWikiContext().getWiki().checkAuth(getXWikiContext());
         if (xWikiUser == null) {
             response = Response.status(Status.UNAUTHORIZED);
         } else {
@@ -215,7 +215,7 @@ public String getNotificationsRSS(String useUserPreferences, String userId, Stri
         String tags, String currentWiki) throws Exception
     {
         // Build the response
-        XWikiUser xWikiUser = getXWikiContext().getWiki().getAuthService().checkAuth(getXWikiContext());
+        XWikiUser xWikiUser = getXWikiContext().getWiki().checkAuth(getXWikiContext());
         DocumentReference userIdDoc = this.documentReferenceResolver.resolve(userId);
         if (xWikiUser == null || !userIdDoc.equals(xWikiUser.getUserReference())) {
             getXWikiContext().getResponse().sendError(HttpServletResponse.SC_UNAUTHORIZED);

@@ -2910,7 +2910,7 @@ public String getCounter(String name)
      */
     public XWikiUser checkAuth() throws XWikiException
     {
-        return this.context.getWiki().getAuthService().checkAuth(this.context);
+        return this.context.getWiki().checkAuth(this.context);
     }
 
     /**
@@ -2925,7 +2925,13 @@ public XWikiUser checkAuth() throws XWikiException
      */
     public XWikiUser checkAuth(String username, String password, String rememberme) throws XWikiException
     {
-        return this.context.getWiki().getAuthService().checkAuth(username, password, rememberme, this.context);
+        XWikiUser user =
+            this.context.getWiki().getAuthService().checkAuth(username, password, rememberme, this.context);
+        if (user.isDisabled(this.context)) {
+            this.context.put(XWikiContext.INACTIVE_USER_REFERENCE, user.getUserReference());
+            user = null;
+        }
+        return user;
     }
 
     /**

@@ -29,6 +29,7 @@
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.model.reference.DocumentReferenceResolver;
 import org.xwiki.model.reference.EntityReferenceSerializer;
+import org.xwiki.model.reference.LocalDocumentReference;
 import org.xwiki.model.reference.WikiReference;
 
 import com.xpn.xwiki.XWiki;
@@ -58,6 +59,9 @@ public class XWikiUser
      */
     public static final String EMAIL_CHECKED_PROPERTY = "email_checked";
 
+    public static final LocalDocumentReference ACCOUNT_VALIDATION_DOCUMENT_REFERENCE =
+        new LocalDocumentReference(XWiki.SYSTEM_SPACE, "AccountValidation");
+
     /**
      * @see com.xpn.xwiki.internal.model.reference.CurrentMixedStringDocumentReferenceResolver
      */

@@ -101,6 +101,16 @@ public class XWikiContext extends Hashtable<Object, Object>
     @Deprecated
     public static final String KEY_LEGACY_VELOCITYCONTEXT = "vcontext";
 
+    /**
+     * The reference of a logged-in inactive user: in such case the context user reference is guest, so we store
+     * the actual logged-in user with that key.
+     *
+     * @since 14.3RC1
+     * @since 13.10.5
+     */
+    @Unstable
+    public static final String INACTIVE_USER_REFERENCE = "inactiveUserReference";
+
     /** Logging helper object. */
     protected static final Logger LOGGER = LoggerFactory.getLogger(XWikiContext.class);
 

@@ -4322,9 +4322,23 @@ public void prepareResources(XWikiContext context)
         }
     }
 
+    /**
+     * Authenticate the user from the context and check if the user is disabled or not.
+     * If the user is disabled, the method returns {@code null} but set the reference of the authenticated user in the
+     * context with the {@link XWikiContext#INACTIVE_USER_REFERENCE} property.
+     *
+     * @param context the context used to authenticate the user.
+     * @return an {@link XWikiUser} if the user is authenticated and enabled, else {@code null}.
+     * @throws XWikiException in case of problem when dealing with the authentication.
+     */
     public XWikiUser checkAuth(XWikiContext context) throws XWikiException
     {
-        return getAuthService().checkAuth(context);
+        XWikiUser user = getAuthService().checkAuth(context);
+        if (user != null && user.isDisabled(context)) {
+            context.put(XWikiContext.INACTIVE_USER_REFERENCE, user.getUserReference());
+            user = null;
+        }
+        return user;
     }
 
     public boolean checkAccess(String action, XWikiDocument doc, XWikiContext context) throws XWikiException
@@ -5864,70 +5878,26 @@ public boolean prepareDocuments(XWikiRequest request, XWikiContext context, Velo
         boolean hasAccess = checkAccess(context.getAction(), doc, context);
 
         XWikiUser user;
+        XWikiUser inactiveUser = null;
+        if (context.getUserReference() == null && context.get(XWikiContext.INACTIVE_USER_REFERENCE) != null) {
+            inactiveUser = new XWikiUser((DocumentReference) context.get(XWikiContext.INACTIVE_USER_REFERENCE));
+        }
         if (context.getUserReference() != null) {
             user = new XWikiUser(context.getUserReference());
         } else {
             user = new XWikiUser(context.getUser());
         }
 
+        if (inactiveUser != null && context.getAction().equals("view")) {
+            this.handleInactiveUserViewAction(inactiveUser, context, reference, vcontext);
+        }
         // We need to check rights before we look for translations
         // Otherwise we don't have the user language
-        if (!hasAccess) {
+        else if (!hasAccess) {
             Object[] args = { doc.getFullName(), user.getUser() };
             setPhonyDocument(reference, context, vcontext);
             throw new XWikiException(XWikiException.MODULE_XWIKI_ACCESS, XWikiException.ERROR_XWIKI_ACCESS_DENIED,
                 "Access to document {0} has been denied to user {1}", null, args);
-        // User is disabled: the mail address is marked as checked
-        } else if (user.isDisabled(context) && user.isEmailChecked(context)) {
-            String action = context.getAction();
-            /*
-             * Allow inactive users to see skins, ressources, SSX, JSX and downloads they could have seen as guest. The
-             * rational behind this behaviour is that inactive users should be able to access the same UI that guests
-             * are used to see, including custom icons, panels, and so on...
-             */
-            if (!((action.equals("skin") && (doc.getSpace().equals("skins") || doc.getSpace().equals("resources")))
-                || ((action.equals("skin") || action.equals("download") || action.equals("ssx") || action.equals("jsx"))
-                    && getRightService().hasAccessLevel("view", XWikiRightService.GUEST_USER_FULLNAME,
-                        doc.getPrefixedFullName(), context))
-                || ((action.equals("view") && doc.getFullName().equals("XWiki.AccountValidation"))))) {
-                Object[] args = { user.getUser() };
-                setPhonyDocument(reference, context, vcontext);
-                throw new XWikiException(XWikiException.MODULE_XWIKI_USER, XWikiException.ERROR_XWIKI_USER_DISABLED,
-                    "User {0} account is disabled", null, args);
-            }
-        // User actually needs to activate his mail address.
-        } else if (user.isDisabled(context) && !user.isEmailChecked(context)) {
-            boolean allow = false;
-            String action = context.getAction();
-            /*
-             * Allow inactive users to see skins, ressources, SSX, JSX and downloads they could have seen as guest. The
-             * rational behind this behaviour is that inactive users should be able to access the same UI that guests
-             * are used to see, including custom icons, panels, and so on...
-             */
-            if ((action.equals("skin") && (doc.getSpace().equals("skins") || doc.getSpace().equals("resources")))
-                || ((action.equals("skin") || action.equals("download") || action.equals("ssx") || action.equals("jsx"))
-                    && getRightService().hasAccessLevel("view", XWikiRightService.GUEST_USER_FULLNAME,
-                        doc.getPrefixedFullName(), context))
-                || ((action.equals("view") && doc.getFullName().equals("XWiki.AccountValidation")))) {
-                allow = true;
-            } else {
-                String allowed = getConfiguration().getProperty("xwiki.inactiveuser.allowedpages", "");
-                if (context.getAction().equals("view") && !allowed.equals("")) {
-                    String[] allowedList = StringUtils.split(allowed, " ,");
-                    for (String element : allowedList) {
-                        if (element.equals(doc.getFullName())) {
-                            allow = true;
-                            break;
-                        }
-                    }
-                }
-            }
-            if (!allow) {
-                Object[] args = { context.getUser() };
-                setPhonyDocument(reference, context, vcontext);
-                throw new XWikiException(XWikiException.MODULE_XWIKI_USER, XWikiException.ERROR_XWIKI_USER_INACTIVE,
-                    "User {0} account is inactive", null, args);
-            }
         }
 
         if (!"skin".equals(context.getAction())
@@ -5969,6 +5939,37 @@ && getRightService().hasAccessLevel("view", XWikiRightService.GUEST_USER_FULLNAM
         return true;
     }
 
+    private void handleInactiveUserViewAction(XWikiUser inactiveUser, XWikiContext context, DocumentReference reference,
+        VelocityContext vcontext) throws XWikiException
+    {
+        if (inactiveUser.isEmailChecked(context)) {
+            Object[] args = { inactiveUser.getUser() };
+            setPhonyDocument(reference, context, vcontext);
+            throw new XWikiException(XWikiException.MODULE_XWIKI_USER, XWikiException.ERROR_XWIKI_USER_DISABLED,
+                "User {0} account is disabled", null, args);
+        } else if (!reference.getLocalDocumentReference().equals(XWikiUser.ACCOUNT_VALIDATION_DOCUMENT_REFERENCE)) {
+            String allowed = getConfiguration().getProperty("xwiki.inactiveuser.allowedpages", "");
+            boolean allow = false;
+            if (!StringUtils.isEmpty(allowed)) {
+                XWikiDocument doc = this.getDocument(reference, context);
+                String[] allowedList = StringUtils.split(allowed, " ,");
+                for (String element : allowedList) {
+                    if (element.equals(doc.getFullName())) {
+                        allow = true;
+                        break;
+                    }
+                }
+            }
+
+            if (!allow) {
+                Object[] args = { inactiveUser.getUser() };
+                setPhonyDocument(reference, context, vcontext);
+                throw new XWikiException(XWikiException.MODULE_XWIKI_USER, XWikiException.ERROR_XWIKI_USER_INACTIVE,
+                    "User {0} account is inactive", null, args);
+            }
+        }
+    }
+
     /**
      * @since 8.3M1
      */

@@ -280,7 +280,9 @@ public boolean checkAccess(String action, XWikiDocument doc, XWikiContext contex
         // implementation, so code that simply want to verify if a user can delete (but is not actually deleting)
         // has to call checkAccess. This happen really often, and this why we should not redirect to login on failed
         // delete, since it would prevent most user to do anything.
-        if (context.getUserReference() == null && !DELETE_ACTION.equals(action) && !LOGIN_ACTION.equals(action)) {
+        // Also we don't show the login if an inactive user logged in.
+        if (context.getUserReference() == null && !DELETE_ACTION.equals(action) && !LOGIN_ACTION.equals(action)
+            && context.get(XWikiContext.INACTIVE_USER_REFERENCE) == null) {
             LOGGER.debug("Redirecting unauthenticated user to login, since it have been denied [{}] on [{}].",
                          right, entityReference);
             showLogin(context);