From 00ff8f9a46058dde1d0edfe38c9fd0b306f80e19 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Vilain Date: Wed, 3 Jul 2013 18:05:34 +0200 Subject: [PATCH] SONAR-4412 Improved error handling --- .../permission/InternalPermissionService.java | 23 ++++++++++++------- .../permission/PermissionChangeQuery.java | 9 ++++---- .../org/sonar/server/user/UserSession.java | 6 ++--- .../app/controllers/api/api_controller.rb | 18 +++++++++++---- .../controllers/api/permissions_controller.rb | 3 ++- .../app/controllers/application_controller.rb | 13 +++++++++++ 6 files changed, 51 insertions(+), 21 deletions(-) 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 7fca7e17a37..25be7f704ba 100644 --- 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 params) { + public void addPermission(final Map params) { changePermission(ADD, params); } @@ -61,6 +58,7 @@ public class InternalPermissionService implements ServerComponent { } private void changePermission(String permissionChange, Map 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 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 cc9175229fb..c55564a0de4 100644 --- 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 f0e67d36e3a..b51f71b0b47 100644 --- 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 ccb9e40b3ad..08a4f4acd49 100644 --- 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 ebff577ef20..a8acbbfc80c 100644 --- 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 4d4eb2e6c07..0d8ebad5232 100644 --- 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 -- 2.39.5