From f50240d53b1f6f67daecfeabb26bd45af0072f55 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Wed, 17 Jul 2013 15:00:36 +0200 Subject: [PATCH] SONAR-4099 Issues & Measures filters should also be sharable only by users who have this permission. --- .../resources/org/sonar/l10n/core.properties | 4 +- .../issue/InternalRubyIssueService.java | 4 ++ .../server/issue/IssueFilterService.java | 43 +++++++++++------ .../app/controllers/api/api_controller.rb | 2 +- .../app/controllers/application_controller.rb | 12 +++-- .../app/controllers/measures_controller.rb | 7 +-- .../views/issues/_filter_shared_form.html.erb | 18 +++---- .../app/views/measures/_shared_form.html.erb | 16 ++++--- .../issue/InternalRubyIssueServiceTest.java | 8 +++- .../server/issue/IssueFilterServiceTest.java | 48 ++++++++++++------- 10 files changed, 104 insertions(+), 58 deletions(-) diff --git a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties b/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties index 478ad0509a8..29a875ae924 100644 --- a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties +++ b/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties @@ -2234,8 +2234,8 @@ global_permissions.admin=System Administration global_permissions.admin.desc=Ability to perform all administration functions for the instance: global configuration and personalization of default dashboards. global_permissions.profileadmin=Quality Profile Administration global_permissions.profileadmin.desc=Ability to perform any action on the quality profiles. -global_permissions.shareDashboard=Dashboard Sharing -global_permissions.shareDashboard.desc=Ability to share dashboards that any user will be able to follow. +global_permissions.shareDashboard=Dashboard And Filter Sharing +global_permissions.shareDashboard.desc=Ability to share dashboards, issue filters and measure filters. global_permissions.scan=Analysis Execution global_permissions.scan.desc=Ability to execute analyses, and to get all settings required to perform the analysis, even the secured ones like the scm account password, the jira account password, and so on. global_permissions.dryRunScan=Local (dry run) Analysis Execution diff --git a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java index 7d657d98751..b96f8658b4c 100644 --- a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java +++ b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java @@ -411,6 +411,10 @@ public class InternalRubyIssueService implements ServerComponent { } } + public boolean canUserShareIssueFilter(){ + return issueFilterService.canShareFilter(UserSession.get()); + } + public String serializeFilterQuery(Map filterQuery) { return issueFilterService.serializeFilterQuery(filterQuery); } diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java index 622607aa66c..a9ae87ab0d3 100644 --- a/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java +++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java @@ -93,19 +93,19 @@ public class IssueFilterService implements ServerComponent { public DefaultIssueFilter save(DefaultIssueFilter issueFilter, UserSession userSession) { String user = getLoggedLogin(userSession); issueFilter.setUser(user); - validateFilter(issueFilter); + validateFilter(issueFilter, userSession); return insertIssueFilter(issueFilter, user); } public DefaultIssueFilter update(DefaultIssueFilter issueFilter, UserSession userSession) { String login = getLoggedLogin(userSession); IssueFilterDto existingFilterDto = findIssueFilterDto(issueFilter.id(), login); - verifyCurrentUserCanModifyFilter(existingFilterDto.toIssueFilter(), login); + verifyCurrentUserCanModifyFilter(existingFilterDto.toIssueFilter(), userSession); verifyCurrentUserCanChangeFilterSharingFilter(issueFilter, existingFilterDto, login); if (!existingFilterDto.getUserLogin().equals(issueFilter.user())) { - verifyCurrentUserCanChangeFilterOwnership(login); + verifyCurrentUserCanChangeFilterOwnership(userSession); } - validateFilter(issueFilter); + validateFilter(issueFilter, userSession); deleteOtherFavoriteFiltersIfFilterBecomeUnshared(existingFilterDto, issueFilter); filterDao.update(IssueFilterDto.toIssueFilter(issueFilter)); return issueFilter; @@ -124,7 +124,7 @@ public class IssueFilterService implements ServerComponent { public DefaultIssueFilter updateFilterQuery(Long issueFilterId, Map filterQuery, UserSession userSession) { String login = getLoggedLogin(userSession); IssueFilterDto issueFilterDto = findIssueFilterDto(issueFilterId, login); - verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), login); + verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), userSession); DefaultIssueFilter issueFilter = issueFilterDto.toIssueFilter(); issueFilter.setData(serializeFilterQuery(filterQuery)); @@ -135,7 +135,7 @@ public class IssueFilterService implements ServerComponent { public void delete(Long issueFilterId, UserSession userSession) { String login = getLoggedLogin(userSession); IssueFilterDto issueFilterDto = findIssueFilterDto(issueFilterId, login); - verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), login); + verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), userSession); deleteFavouriteIssueFilters(issueFilterDto); filterDao.delete(issueFilterId); @@ -144,9 +144,11 @@ public class IssueFilterService implements ServerComponent { public DefaultIssueFilter copy(Long issueFilterIdToCopy, DefaultIssueFilter issueFilter, UserSession userSession) { String login = getLoggedLogin(userSession); IssueFilterDto issueFilterDtoToCopy = findIssueFilterDto(issueFilterIdToCopy, login); + // Copy of filter should not be shared + issueFilterDtoToCopy.setShared(false); issueFilter.setUser(login); issueFilter.setData(issueFilterDtoToCopy.getData()); - validateFilter(issueFilter); + validateFilter(issueFilter, userSession); return insertIssueFilter(issueFilter, login); } @@ -198,9 +200,13 @@ public class IssueFilterService implements ServerComponent { return issueFilterDto; } + public boolean canShareFilter(UserSession userSession){ + return userSession.hasGlobalPermission(Permission.DASHBOARD_SHARING); + } + String getLoggedLogin(UserSession userSession) { String user = userSession.login(); - if (!userSession.isLoggedIn() && user != null) { + if (!userSession.isLoggedIn()) { throw new UnauthorizedException("User is not logged in"); } return user; @@ -212,8 +218,8 @@ public class IssueFilterService implements ServerComponent { } } - private void verifyCurrentUserCanModifyFilter(DefaultIssueFilter issueFilter, String user) { - if (!issueFilter.user().equals(user) && !isAdmin(user)) { + private void verifyCurrentUserCanModifyFilter(DefaultIssueFilter issueFilter, UserSession userSession) { + if (!issueFilter.user().equals(userSession.login()) && !isAdmin(userSession)) { throw new ForbiddenException("User is not authorized to modify this filter"); } } @@ -224,13 +230,19 @@ public class IssueFilterService implements ServerComponent { } } - private void verifyCurrentUserCanChangeFilterOwnership(String user) { - if (!isAdmin(user)) { + private void verifyCurrentUserCanChangeFilterOwnership(UserSession userSession) { + if (!isAdmin(userSession)) { throw new ForbiddenException("User is not authorized to change the owner of this filter"); } } - private void validateFilter(final DefaultIssueFilter issueFilter) { + private void verifyCurrentUserCanShareFilter(DefaultIssueFilter issueFilter, UserSession userSession) { + if (issueFilter.shared() && !canShareFilter(userSession)) { + throw new ForbiddenException("User is not authorized to share this filter"); + } + } + + private void validateFilter(final DefaultIssueFilter issueFilter, UserSession userSession) { List userFilters = selectUserIssueFilters(issueFilter.user()); IssueFilterDto userFilterSameName = findFilterWithSameName(userFilters, issueFilter.name()); if (userFilterSameName != null && !userFilterSameName.getId().equals(issueFilter.id())) { @@ -242,6 +254,7 @@ public class IssueFilterService implements ServerComponent { if (sharedFilterWithSameName != null && !sharedFilterWithSameName.getId().equals(issueFilter.id())) { throw new BadRequestException("Other users already share filters with the same name"); } + verifyCurrentUserCanShareFilter(issueFilter, userSession); } } @@ -308,8 +321,8 @@ public class IssueFilterService implements ServerComponent { })); } - private boolean isAdmin(String user) { - return authorizationDao.selectGlobalPermissions(user).contains(Permission.SYSTEM_ADMIN.key()); + private boolean isAdmin(UserSession userSession) { + return userSession.hasGlobalPermission(Permission.SYSTEM_ADMIN); } private IssueFilterResult createIssueFilterResult(IssueQueryResult issueQueryResult, IssueQuery issueQuery) { 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 80976c8ae09..2e09ab554ea 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 @@ -78,7 +78,7 @@ class Api::ApiController < ApplicationController # # - def render_native_access_denied + def render_native_access_denied(exception) render_access_denied end 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 869c6b61458..fb7fa6d6b2b 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 @@ -174,17 +174,21 @@ class ApplicationController < ActionController::Base render :text => message, :status => exception.httpCode end - def render_native_access_denied - access_denied + def render_native_access_denied(exception) + if request.xhr? + render_server_exception(exception) + else + access_denied + end 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::UnauthorizedException - render_native_access_denied + render_native_access_denied(error.cause) elsif error.cause.java_kind_of? Java::OrgSonarServerExceptions::ForbiddenException - render_native_access_denied + render_native_access_denied(error.cause) elsif error.cause.java_kind_of? Java::OrgSonarServerExceptions::HttpException render_server_exception(error.cause) else diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb index 078f69f9f27..e83daf4af9a 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb @@ -85,7 +85,7 @@ class MeasuresController < ApplicationController end @filter.name=params[:name] @filter.description=params[:description] - @filter.shared=(params[:shared]=='true') + @filter.shared=(params[:shared]=='true') && has_role?(:shareDashboard) @filter.data=URI.unescape(params[:data]) if @filter.save current_user.favourited_measure_filters<<@filter if add_to_favourites @@ -138,7 +138,7 @@ class MeasuresController < ApplicationController @filter.name=params[:name] @filter.description=params[:description] - @filter.shared=(params[:shared]=='true') + @filter.shared=(params[:shared]=='true') && has_role?(:shareDashboard) if has_role?(:admin) && params[:owner] @filter.user = User.find_by_login(params[:owner]) end @@ -171,7 +171,8 @@ class MeasuresController < ApplicationController target.name=params[:name] target.description=params[:description] target.user_id=current_user.id - target.shared=(params[:shared]=='true') + # Copy of filter should never be shared + target.shared=false target.data=source.data if target.save current_user.favourited_measure_filters << target diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_filter_shared_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_filter_shared_form.html.erb index d595611f70e..5945781f885 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_filter_shared_form.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_filter_shared_form.html.erb @@ -20,12 +20,14 @@ <% else %> <% end %> -
- <% if !@filter || @filter.user == current_user.login %> - - /> - <% else %> - - <% end %> -
+ <% if Internal.issues.canUserShareIssueFilter() %> +
+ <% if !@filter || @filter.user == current_user.login %> + + /> + <% else %> + + <% end %> +
+ <% end %> \ No newline at end of file diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_shared_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_shared_form.html.erb index f5a6ed43dbe..6b551d48a38 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_shared_form.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_shared_form.html.erb @@ -19,12 +19,14 @@ <%= user_select_tag('owner', :html_id => 'select-filter-owner', :selected_user => @filter.user) -%> <% end %> - <% if @filter.user_id.nil? || @filter.user_id == current_user.id %> -
- - /> -
- <% else %> - + <% if has_role?(:shareDashboard) %> + <% if @filter.user_id.nil? || @filter.user_id == current_user.id %> +
+ + /> +
+ <% else %> + + <% end %> <% end %> \ No newline at end of file diff --git a/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java index 95dcdb4aac2..82849fe0371 100644 --- a/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java +++ b/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java @@ -531,13 +531,19 @@ public class InternalRubyIssueServiceTest { } @Test - public void should_check_is_user_is_authorized_to_see_issue_filter() { + public void should_check_if_user_is_authorized_to_see_issue_filter() { DefaultIssueFilter issueFilter = new DefaultIssueFilter(); service.isUserAuthorized(issueFilter); verify(issueFilterService).getLoggedLogin(any(UserSession.class)); verify(issueFilterService).verifyCurrentUserCanReadFilter(eq(issueFilter), anyString()); } + @Test + public void should_check_if_user_can_share_issue_filter(){ + service.canUserShareIssueFilter(); + verify(issueFilterService).canShareFilter(any(UserSession.class)); + } + @Test public void should_execute_bulk_change() { Map params = newHashMap(); diff --git a/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java index 49a5cbd5da2..16f5948ae1b 100644 --- a/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java +++ b/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java @@ -74,10 +74,7 @@ public class IssueFilterServiceTest { @Before public void before() { - userSession = mock(UserSession.class); - when(userSession.isLoggedIn()).thenReturn(true); - when(userSession.userId()).thenReturn(10); - when(userSession.login()).thenReturn("john"); + userSession = MockUserSession.create().setLogin("john"); issueFilterDao = mock(IssueFilterDao.class); issueFilterFavouriteDao = mock(IssueFilterFavouriteDao.class); @@ -131,7 +128,7 @@ public class IssueFilterServiceTest { @Test public void should_not_find_by_id_if_not_logged() { - when(userSession.isLoggedIn()).thenReturn(false); + UserSession userSession = MockUserSession.create().setLogin(null); try { service.find(1L, userSession); fail(); @@ -165,7 +162,7 @@ public class IssueFilterServiceTest { @Test public void should_not_find_by_user_if_not_logged() { - when(userSession.isLoggedIn()).thenReturn(false); + UserSession userSession = MockUserSession.create().setLogin(null); try { service.findByUser(userSession); fail(); @@ -195,7 +192,7 @@ public class IssueFilterServiceTest { @Test public void should_not_save_if_not_logged() { - when(userSession.isLoggedIn()).thenReturn(false); + UserSession userSession = MockUserSession.create().setLogin(null); try { DefaultIssueFilter issueFilter = new DefaultIssueFilter().setName("My Issue"); service.save(issueFilter, userSession); @@ -244,20 +241,35 @@ public class IssueFilterServiceTest { } @Test - public void should_update_sharing() { - when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Filter").setShared(true).setUserLogin("john")); + public void should_have_permission_to_share_filter() { + UserSession userSession = MockUserSession.create().setLogin("john").setPermissions(Permission.DASHBOARD_SHARING); + when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Filter").setShared(false).setUserLogin("john")); - DefaultIssueFilter result = service.update(new DefaultIssueFilter().setId(1L).setName("My Filter").setShared(false).setUser("john"), userSession); - assertThat(result.shared()).isFalse(); + DefaultIssueFilter result = service.update(new DefaultIssueFilter().setId(1L).setName("My Filter").setShared(true).setUser("john"), userSession); + assertThat(result.shared()).isTrue(); verify(issueFilterDao).update(any(IssueFilterDto.class)); } + @Test + public void should_not_share_filter_if_no_permission() { + UserSession userSession = MockUserSession.create().setLogin("john").setPermissions(); + when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Filter").setShared(false).setUserLogin("john")); + + try { + service.update(new DefaultIssueFilter().setId(1L).setName("My Filter").setShared(true).setUser("john"), userSession); + fail(); + } catch (Exception e) { + assertThat(e).isInstanceOf(ForbiddenException.class).hasMessage("User is not authorized to share this filter"); + } + verify(issueFilterDao, never()).update(any(IssueFilterDto.class)); + } + @Test public void should_not_update_sharing_if_not_owner() { // John is admin and want to change arthur filter sharing + UserSession userSession = MockUserSession.create().setLogin("john").setPermissions(Permission.SYSTEM_ADMIN); when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("Arthur Filter").setShared(true).setUserLogin("arthur")); - when(authorizationDao.selectGlobalPermissions("john")).thenReturn(newArrayList(Permission.SYSTEM_ADMIN.key())); try { service.update(new DefaultIssueFilter().setId(1L).setName("Arthur Filter").setShared(false).setUser("john"), userSession); @@ -297,7 +309,7 @@ public class IssueFilterServiceTest { @Test public void should_update_other_shared_filter_if_admin() { - when(authorizationDao.selectGlobalPermissions("john")).thenReturn(newArrayList(Permission.SYSTEM_ADMIN.key())); + UserSession userSession = MockUserSession.create().setLogin("john").setPermissions(Permission.SYSTEM_ADMIN, Permission.DASHBOARD_SHARING); when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Old Filter").setDescription("Old description").setUserLogin("arthur").setShared(true)); DefaultIssueFilter result = service.update(new DefaultIssueFilter().setId(1L).setName("My New Filter").setDescription("New description").setShared(true), userSession); @@ -368,12 +380,13 @@ public class IssueFilterServiceTest { IssueFilterDto sharedFilter = new IssueFilterDto().setId(1L).setName("My filter").setUserLogin("former.owner").setShared(true); IssueFilterDto expectedDto = new IssueFilterDto().setName("My filter").setUserLogin("new.owner").setShared(true); - when(authorizationDao.selectGlobalPermissions(currentUser)).thenReturn(newArrayList(Permission.SYSTEM_ADMIN.key())); + UserSession userSession = MockUserSession.create().setLogin(currentUser).setPermissions(Permission.SYSTEM_ADMIN, Permission.DASHBOARD_SHARING); + when(issueFilterDao.selectById(1L)).thenReturn(sharedFilter); when(issueFilterDao.selectSharedFilters()).thenReturn(Lists.newArrayList(sharedFilter)); DefaultIssueFilter issueFilter = new DefaultIssueFilter().setId(1L).setName("My filter").setShared(true).setUser("new.owner"); - service.update(issueFilter, MockUserSession.create().setUserId(1).setLogin(currentUser)); + service.update(issueFilter, userSession); verify(issueFilterDao).update(argThat(Matches.filter(expectedDto))); } @@ -432,7 +445,7 @@ public class IssueFilterServiceTest { @Test public void should_delete_shared_filter_if_user_is_admin() { - when(authorizationDao.selectGlobalPermissions("john")).thenReturn(newArrayList(Permission.SYSTEM_ADMIN.key())); + UserSession userSession = MockUserSession.create().setLogin("john").setPermissions(Permission.SYSTEM_ADMIN); when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Issues").setUserLogin("arthur").setShared(true)); service.delete(1L, userSession); @@ -489,6 +502,7 @@ public class IssueFilterServiceTest { DefaultIssueFilter result = service.copy(1L, issueFilter, userSession); assertThat(result.name()).isEqualTo("Copy Of My Issue"); assertThat(result.user()).isEqualTo("john"); + assertThat(result.shared()).isFalse(); verify(issueFilterDao).insert(any(IssueFilterDto.class)); } @@ -535,7 +549,7 @@ public class IssueFilterServiceTest { @Test public void should_not_find_favourite_issue_filter_if_not_logged() { - when(userSession.isLoggedIn()).thenReturn(false); + UserSession userSession = MockUserSession.create().setLogin(null); try { service.findFavoriteFilters(userSession); -- 2.39.5