SONAR-4412 Improved error handling

diff --git a/sonar-server/src/main/java/org/sonar/server/permission/InternalPermissionService.java b/sonar-server/src/main/java/org/sonar/server/permission/InternalPermissionService.java
index 7fca7e17a37cc7be2deded8c4ba3afa338edcd3c..25be7f704ba2cadbbbad67ee3f8e561a8790e725 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/permission/InternalPermissionService.java
+++ b/sonar-server/src/main/java/org/sonar/server/permission/InternalPermissionService.java
@@ -24,11 +24,8 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.sonar.api.ServerComponent;
 import org.sonar.api.security.DefaultGroups;
-import org.sonar.core.user.GroupRoleDto;
-import org.sonar.core.user.Permission;
-import org.sonar.core.user.RoleDao;
-import org.sonar.core.user.UserDao;
-import org.sonar.core.user.UserRoleDto;
+import org.sonar.core.user.*;
+import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.user.UserSession;
 
 import java.util.List;
@@ -52,7 +49,7 @@ public class InternalPermissionService implements ServerComponent {
     this.userDao = userDao;
   }
 
-  public void addPermission(Map<String, Object> params) {
+  public void addPermission(final Map<String, Object> params) {
     changePermission(ADD, params);
   }
 
@@ -61,6 +58,7 @@ public class InternalPermissionService implements ServerComponent {
   }
 
   private void changePermission(String permissionChange, Map<String, Object> params) {
+    UserSession.get().checkLoggedIn();
     UserSession.get().checkGlobalPermission(Permission.SYSTEM_ADMIN);
     PermissionChangeQuery permissionChangeQuery = PermissionChangeQuery.buildFromParams(params);
     permissionChangeQuery.validate();
@@ -108,14 +106,23 @@ public class InternalPermissionService implements ServerComponent {
   }
 
   private Long getTargetedUser(String userLogin) {
-    return userDao.selectActiveUserByLogin(userLogin).getId();
+    UserDto user = userDao.selectActiveUserByLogin(userLogin);
+    if(user == null) {
+      throw new BadRequestException("User " + userLogin + " does not exist");
+    }
+    return user.getId();
   }
 
   private Long getTargetedGroup(String group) {
     if (DefaultGroups.isAnyone(group)) {
       return null;
+    } else {
+      GroupDto groupDto = userDao.selectGroupByName(group);
+      if(groupDto == null) {
+        throw new BadRequestException("Group " + group + " does not exist");
+      }
+      return groupDto.getId();
     }
-    return userDao.selectGroupByName(group).getId();
   }
 
   private boolean shouldSkipPermissionChange(String operation, List<String> existingPermissions, String role) {

diff --git a/sonar-server/src/main/java/org/sonar/server/permission/PermissionChangeQuery.java b/sonar-server/src/main/java/org/sonar/server/permission/PermissionChangeQuery.java
index cc9175229fb96c2d30fb09df6b3fd298b3bb017d..c55564a0de4492905c2c662202c4a2a534d73852 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/permission/PermissionChangeQuery.java
+++ b/sonar-server/src/main/java/org/sonar/server/permission/PermissionChangeQuery.java
@@ -22,6 +22,7 @@ package org.sonar.server.permission;
 
 import org.apache.commons.lang.StringUtils;
 import org.sonar.core.user.Permission;
+import org.sonar.server.exceptions.BadRequestException;
 
 import java.util.Map;
 
@@ -47,16 +48,16 @@ public class PermissionChangeQuery {
 
   public void validate() {
     if (StringUtils.isBlank(role)) {
-      throw new IllegalArgumentException("Missing role parameter");
+      throw new BadRequestException("Missing role parameter");
     }
     if (StringUtils.isBlank(user) && StringUtils.isBlank(group)) {
-      throw new IllegalArgumentException("Missing user or group parameter");
+      throw new BadRequestException("Missing user or group parameter");
     }
     if (StringUtils.isNotBlank(user) && StringUtils.isNotBlank(group)) {
-      throw new IllegalArgumentException("Only one of user or group parameter should be provided");
+      throw new BadRequestException("Only one of user or group parameter should be provided");
     }
     if (!Permission.allGlobal().keySet().contains(role)) {
-      throw new IllegalArgumentException("Invalid role key " + role);
+      throw new BadRequestException("Invalid role key " + role);
     }
   }
 

diff --git a/sonar-server/src/main/java/org/sonar/server/user/UserSession.java b/sonar-server/src/main/java/org/sonar/server/user/UserSession.java
index f0e67d36e3a35609f8f11966d8a39bce3c27b0e4..b51f71b0b47bc3bdf527a44b72f9e10420017caf 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/user/UserSession.java
+++ b/sonar-server/src/main/java/org/sonar/server/user/UserSession.java
@@ -88,7 +88,7 @@ public class UserSession {
 
   public UserSession checkLoggedIn() {
     if (login == null) {
-      throw new UnauthorizedException();
+      throw new UnauthorizedException("Authentication is required");
     }
     return this;
   }
@@ -98,7 +98,7 @@ public class UserSession {
    */
   public UserSession checkGlobalPermission(Permission permission) {
     if (!hasGlobalPermission(permission)) {
-      throw new ForbiddenException();
+      throw new ForbiddenException("Insufficient privileges");
     }
     return this;
   }
@@ -117,7 +117,7 @@ public class UserSession {
       for (String permissionKey : permissionKeys) {
         Permission perm = Permission.allGlobal().get(permissionKey);
         if (perm == null) {
-          LOG.warn("Ignoring unknow permission {} for user {}", permissionKey, login);
+          LOG.warn("Ignoring unknown permission {} for user {}", permissionKey, login);
         }
         else {
           permissions.add(perm);

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/api_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/api_controller.rb
index ccb9e40b3ad309e7c50377efc9cd8cc636f96e1c..08a4f4acd497b0ffbd676713573d16f37dda4e09 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/api_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/api_controller.rb
@@ -81,11 +81,7 @@ class Api::ApiController < ApplicationController
   def render_error(exception, status=500)
     message = exception.respond_to?('message') ? Api::Utils.exception_message(exception, :backtrace => true) : exception.to_s
     java_facade.logError("Fail to render: #{request.url}\n#{message}") if status==500
-    respond_to do |format|
-      format.json { render :json => error_to_json(status, message), :status => status }
-      format.xml { render :xml => error_to_xml(status, message), :status => status }
-      format.text { render :text => message, :status => status }
-    end
+    render_response(status, message)
   end
 
   def render_not_found(exception)
@@ -106,6 +102,18 @@ class Api::ApiController < ApplicationController
     render_error(message, 200)
   end
 
+  def render_server_exception(exception)
+    render_response(exception.httpCode, exception.getMessage)
+  end
+
+  def render_response(status, message)
+    respond_to do |format|
+      format.json { render :json => error_to_json(status, message), :status => status }
+      format.xml { render :xml => error_to_xml(status, message), :status => status }
+      format.text { render :text => message, :status => status }
+    end
+  end
+
   def error_to_json(status, message=nil)
     hash={:err_code => status}
     hash[:err_msg]=message if message

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/permissions_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/permissions_controller.rb
index ebff577ef20cec4d441f0b041c33b8822b7c6d8b..a8acbbfc80c5081c07415e168fb921895858b639 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/permissions_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/permissions_controller.rb
@@ -43,7 +43,8 @@ class Api::PermissionsController < Api::ApiController
   #
   def add
     verify_post_request
-    require_parameters :permission, (params[:user].nil? ? :group : :user)
+    require_parameters :permission
+    require_one_of :group, :user
     Internal.permissions.addPermission(params)
     hash = {:user => params[:user], :group => params[:group], :permission => params[:permission]}
     respond_to do |format|

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb
index 4d4eb2e6c071f05f76aa50e9f6ad21562875da4b..0d8ebad52322f5faf43e7d7965ea7ed5130731ae 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb
@@ -143,6 +143,13 @@ class ApplicationController < ActionController::Base
     end
   end
 
+  # since 3.7
+  def require_one_of(*keys)
+    if keys.count {|key| !params[key].blank?} == 0
+      bad_request("One of the following parameters should be provided: #{keys.join(',')}")
+    end
+  end
+
   # since 3.3
   def verify_post_request
     bad_request('Not a POST request') unless request.post?
@@ -162,9 +169,15 @@ class ApplicationController < ActionController::Base
     render :text => message, :status => 400
   end
 
+  def render_server_exception(exception)
+    render :text => exception.getMessage, :status => exception.httpCode
+  end
+
   def render_native_exception(error)
     if error.cause.java_kind_of? Java::JavaLang::IllegalArgumentException
       render_bad_request(error.cause.getMessage)
+    elsif error.cause.java_kind_of? Java::OrgSonarServerExceptions::HttpException
+      render_server_exception(error.cause)
     else
       render_error(error)
     end