Merge pull request #787 from zanata/rhbz903964-better-error-msg

rhbz903964 - produce better error message when permission check failed

docs/release-notes.md

@@ -30,6 +30,7 @@ Example usage in html file: `<link rel="shortcut icon" href="#{assets['img/logo/
 * [1207426](https://bugzilla.redhat.com/show_bug.cgi?id=1207426) - Update request to join group/language page
 * [1165939](https://bugzilla.redhat.com/show_bug.cgi?id=1165939) - The Groups actions panel should not show for a normal user
 * [1205512](https://bugzilla.redhat.com/show_bug.cgi?id=1205512) - Run validation in editor document list is disabled when it should not be
+* [903964](https://bugzilla.redhat.com/show_bug.cgi?id=903964) - Error message not propagated to client when push fails
 
 -----------------------
 

zanata-model/src/main/java/org/zanata/model/HAccount.java

@@ -71,7 +71,8 @@
 @ToString(callSuper = true, of = "username")
 @EqualsAndHashCode(callSuper = true, of = { "enabled", "passwordHash",
         "username", "apiKey" })
-public class HAccount extends ModelEntityBase implements Serializable {
+public class HAccount extends ModelEntityBase implements Serializable,
+        HasUserFriendlyToString {
     private static final long serialVersionUID = 1L;
 
     private String username;
@@ -169,4 +170,9 @@ public HAccount getMergedInto() {
     public Map<String, HAccountOption> getEditorOptions() {
         return editorOptions;
     }
+
+    @Override
+    public String userFriendlyToString() {
+        return String.format("Account(username=%s, enabled=%s, roles=%s)", getUsername(), isEnabled(), getRoles());
+    }
 }

zanata-model/src/main/java/org/zanata/model/HAccountRole.java

@@ -43,7 +43,7 @@
 @Entity
 @Setter
 @TypeDef(name = "roleType", typeClass = RoleTypeType.class)
-public class HAccountRole implements Serializable {
+public class HAccountRole implements Serializable, HasUserFriendlyToString {
     private static final long serialVersionUID = 9177366120789064801L;
 
     private Integer id;
@@ -85,6 +85,11 @@ public RoleType getRoleType() {
         return roleType;
     }
 
+    @Override
+    public String userFriendlyToString() {
+        return getName();
+    }
+
     public enum RoleType {
         AUTO, MANUAL;
 

zanata-model/src/main/java/org/zanata/model/HIterationGroup.java

@@ -48,7 +48,8 @@
 @Setter
 @Getter
 @Access(AccessType.FIELD)
-public class HIterationGroup extends SlugEntityBase implements HasEntityStatus {
+public class HIterationGroup extends SlugEntityBase implements HasEntityStatus,
+        HasUserFriendlyToString {
     private static final long serialVersionUID = 5682522115222479842L;
 
     @Size(max = 80)
@@ -95,4 +96,10 @@ public void removeMaintainer(HPerson maintainer) {
     public void addProjectIteration(HProjectIteration iteration) {
         this.getProjectIterations().add(iteration);
     }
+
+    @Override
+    public String userFriendlyToString() {
+        return String.format("Version group(slug=%s, name=%s, status=%s",
+                getSlug(), getName(), getStatus());
+    }
 }

zanata-model/src/main/java/org/zanata/model/HLocale.java

@@ -58,7 +58,8 @@
 @ToString(of = { "localeId" }, doNotUseGetters = true)
 @EqualsAndHashCode(callSuper = false, of = { "localeId" },
         doNotUseGetters = true)
-public class HLocale extends ModelEntityBase implements Serializable {
+public class HLocale extends ModelEntityBase implements Serializable,
+        HasUserFriendlyToString {
     private static final long serialVersionUID = 1L;
     private @Nonnull
     LocaleId localeId;
@@ -157,4 +158,9 @@ public ULocale asULocale() {
         return new ULocale(this.localeId.getId());
     }
 
+    @Override
+    public String userFriendlyToString() {
+        return String.format("Locale(id=%s, name=%s)", getLocaleId(),
+                retrieveDisplayName());
+    }
 }

zanata-model/src/main/java/org/zanata/model/HLocaleMember.java

@@ -47,7 +47,7 @@
 @Table(name = "HLocale_Member")
 @Setter
 @NoArgsConstructor
-public class HLocaleMember implements Serializable {
+public class HLocaleMember implements Serializable, HasUserFriendlyToString {
     private static final long serialVersionUID = 1L;
 
     private HLocaleMemberPk id = new HLocaleMemberPk();
@@ -56,13 +56,20 @@ public class HLocaleMember implements Serializable {
     private boolean isReviewer;
     private boolean isTranslator;
 
+    private transient String userFriendlyToString;
+
     public HLocaleMember(HPerson person, HLocale supportedLanguage,
             boolean isTranslator, boolean isReviewer, boolean isCoordinator) {
         id.setPerson(person);
         id.setSupportedLanguage(supportedLanguage);
         setTranslator(isTranslator);
         setReviewer(isReviewer);
         setCoordinator(isCoordinator);
+        userFriendlyToString =
+                String.format(
+                        "Locale membership(person=%s, isTranslator=%s, isReviewer=%s, isCoordinator=%s)",
+                        person.getName(), isTranslator, isReviewer,
+                        isCoordinator);
     }
 
     @EmbeddedId
@@ -99,6 +106,11 @@ public HLocale getSupportedLanguage() {
         return id.getSupportedLanguage();
     }
 
+    @Override
+    public String userFriendlyToString() {
+        return userFriendlyToString;
+    }
+
     @Embeddable
     @Setter
     @AllArgsConstructor

zanata-model/src/main/java/org/zanata/model/HProject.java

@@ -97,7 +97,7 @@
 @Indexed
 @ToString(callSuper = true, of = "name")
 public class HProject extends SlugEntityBase implements Serializable,
-        HasEntityStatus {
+        HasEntityStatus, HasUserFriendlyToString {
     private static final long serialVersionUID = 1L;
 
     @Size(max = 80)
@@ -185,4 +185,10 @@ public void addMaintainer(HPerson maintainer) {
         this.getMaintainers().add(maintainer);
         maintainer.getMaintainerProjects().add(this);
     }
+
+    @Override
+    public String userFriendlyToString() {
+        return String.format("Project(name=%s, slug=%s, status=%s)", getName(),
+                getSlug(), getStatus());
+    }
 }

zanata-model/src/main/java/org/zanata/model/HProjectIteration.java

@@ -87,7 +87,8 @@
 @NoArgsConstructor
 @ToString(callSuper = true, of = { "project" })
 public class HProjectIteration extends SlugEntityBase implements
-        Iterable<DocumentWithId>, HasEntityStatus, IsEntityWithType {
+        Iterable<DocumentWithId>, HasEntityStatus, IsEntityWithType,
+        HasUserFriendlyToString {
     private static final long serialVersionUID = 182037127575991478L;
 
     @ManyToOne
@@ -173,4 +174,10 @@ public Boolean getRequireTranslationReview() {
         }
         return requireTranslationReview;
     }
+
+    @Override
+    public String userFriendlyToString() {
+        return String.format("Project version(slug=%s, status=%s)", getSlug(),
+                getStatus());
+    }
 }

zanata-model/src/main/java/org/zanata/model/HasUserFriendlyToString.java

@@ -0,0 +1,36 @@
+/*
+ * Copyright 2015, Red Hat, Inc. and individual contributors as indicated by the
+ * @author tags. See the copyright.txt file in the distribution for a full
+ * listing of individual contributors.
+ *
+ * This is free software; you can redistribute it and/or modify it under the
+ * terms of the GNU Lesser General Public License as published by the Free
+ * Software Foundation; either version 2.1 of the License, or (at your option)
+ * any later version.
+ *
+ * This software is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this software; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA, or see the FSF
+ * site: http://www.fsf.org.
+ */
+package org.zanata.model;
+
+/**
+ * Will give a user friendly string representation of the object. All targets in
+ * SecurityFunctions should implement this interface so that when the permission
+ * check fail and we throw an AuthorizationException, the message will be more
+ * meaningful.
+ *
+ * @see org.jboss.seam.security.SecurityFunctions
+ * @see org.zanata.security.ZanataIdentity#checkPermission(java.lang.String,
+ *      java.lang.Object...)
+ */
+public interface HasUserFriendlyToString {
+
+    String userFriendlyToString();
+}

zanata-war/src/main/java/org/zanata/rest/AuthorizationExceptionMapper.java

@@ -12,7 +12,8 @@ public class AuthorizationExceptionMapper implements ExceptionMapper<Authorizati
 
     @Override
     public Response toResponse(AuthorizationException exception) {
-        return Response.status(Status.UNAUTHORIZED).build();
+        return Response.status(Status.FORBIDDEN)
+                .entity(exception.getMessage()).build();
     }
 
 }

zanata-war/src/main/java/org/zanata/security/ZanataIdentity.java

@@ -20,6 +20,7 @@
  */
 package org.zanata.security;
 
+import java.util.List;
 import javax.annotation.Nullable;
 import javax.security.auth.login.LoginContext;
 import javax.security.auth.login.LoginException;
@@ -32,6 +33,7 @@
 import org.jboss.seam.annotations.intercept.BypassInterceptors;
 import org.jboss.seam.contexts.Contexts;
 import org.jboss.seam.core.Events;
+import org.jboss.seam.security.AuthorizationException;
 import org.jboss.seam.security.Configuration;
 import org.jboss.seam.security.Identity;
 import org.jboss.seam.security.NotLoggedInException;
@@ -41,6 +43,7 @@
 import com.google.common.collect.Lists;
 import org.zanata.events.Logout;
 import org.zanata.model.HAccount;
+import org.zanata.model.HasUserFriendlyToString;
 import org.zanata.security.permission.MultiTargetList;
 import org.zanata.util.ServiceLocator;
 
@@ -141,12 +144,12 @@ public boolean hasPermission(String name, String action, Object... arg) {
         if (result) {
             if (log.isDebugEnabled()) {
                 log.debug("ALLOWED hasPermission({}, {}, {}) for user {}",
-                        name, action, arg, getAccountUsername());
+                        name, action, Lists.newArrayList(arg), getAccountUsername());
             }
         } else {
             if (log.isDebugEnabled()) {
                 log.debug("DENIED hasPermission({}, {}, {}) for user {}",
-                        name, action, arg, getAccountUsername());
+                        name, action, Lists.newArrayList(arg), getAccountUsername());
             }
         }
         log.trace("EXIT hasPermission(): {}", result);
@@ -182,7 +185,27 @@ public boolean hasPermission(String action, Object... targets) {
      *             if logged in but not authorised
      */
     public void checkPermission(String action, Object... targets) {
-        super.checkPermission(MultiTargetList.fromTargets(targets), action);
+        try {
+            super.checkPermission(MultiTargetList.fromTargets(targets), action);
+        } catch (AuthorizationException exception) {
+            // try to produce a better than default error message
+            List<String> meaningfulTargets = Lists.newArrayList();
+            for (Object target : targets) {
+                if (target instanceof HasUserFriendlyToString) {
+                    String targetString = ((HasUserFriendlyToString) target).userFriendlyToString();
+                    meaningfulTargets.add(targetString);
+                } else {
+                    log.warn(
+                            "target [{}] may not have user friendly string representation",
+                            target.getClass());
+                    meaningfulTargets.add(target.toString());
+                }
+            }
+            throw new AuthorizationException(
+                    String.format(
+                            "Failed to obtain permission('%s') with following facts(%s)",
+                            action, meaningfulTargets));
+        }
     }
 
     @Override

zanata-war/src/test/java/org/zanata/rest/compat/AccountRawCompatibilityITCase.java

@@ -235,7 +235,7 @@ public void putAccountXmlUnauthorized() throws Exception {
         Response putResponse = accountClient.put(a);
 
         // Assert initial put
-        assertThat(putResponse.getStatus(), is(Status.UNAUTHORIZED.getStatusCode()));
+        assertThat(putResponse.getStatus(), is(Status.FORBIDDEN.getStatusCode()));
         releaseConnection(putResponse);
     }