From: Julien Lancelot Date: Wed, 17 Jul 2013 15:40:43 +0000 (+0200) Subject: SONAR-2474 When sharing a filter, the owner (and not the current user or previous... X-Git-Tag: 3.7~71 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=20cac9a061e03e3d6fbaee936b46609c2f3f5569;p=sonarqube.git SONAR-2474 When sharing a filter, the owner (and not the current user or previous owner) should have the sharing permission --- 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 b96f8658b4c..6b092d4a65f 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 @@ -412,7 +412,7 @@ public class InternalRubyIssueService implements ServerComponent { } public boolean canUserShareIssueFilter(){ - return issueFilterService.canShareFilter(UserSession.get()); + return issueFilterService.canShareFilter(UserSession.get().login()); } public String serializeFilterQuery(Map 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 a9ae87ab0d3..cff4e165ce9 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, userSession); + validateFilter(issueFilter); return insertIssueFilter(issueFilter, user); } public DefaultIssueFilter update(DefaultIssueFilter issueFilter, UserSession userSession) { String login = getLoggedLogin(userSession); IssueFilterDto existingFilterDto = findIssueFilterDto(issueFilter.id(), login); - verifyCurrentUserCanModifyFilter(existingFilterDto.toIssueFilter(), userSession); + verifyCurrentUserCanModifyFilter(existingFilterDto.toIssueFilter(), login); verifyCurrentUserCanChangeFilterSharingFilter(issueFilter, existingFilterDto, login); if (!existingFilterDto.getUserLogin().equals(issueFilter.user())) { - verifyCurrentUserCanChangeFilterOwnership(userSession); + verifyCurrentUserCanChangeFilterOwnership(login); } - validateFilter(issueFilter, userSession); + validateFilter(issueFilter); 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(), userSession); + verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), login); 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(), userSession); + verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), login); deleteFavouriteIssueFilters(issueFilterDto); filterDao.delete(issueFilterId); @@ -148,7 +148,7 @@ public class IssueFilterService implements ServerComponent { issueFilterDtoToCopy.setShared(false); issueFilter.setUser(login); issueFilter.setData(issueFilterDtoToCopy.getData()); - validateFilter(issueFilter, userSession); + validateFilter(issueFilter); return insertIssueFilter(issueFilter, login); } @@ -200,8 +200,8 @@ public class IssueFilterService implements ServerComponent { return issueFilterDto; } - public boolean canShareFilter(UserSession userSession){ - return userSession.hasGlobalPermission(Permission.DASHBOARD_SHARING); + public boolean canShareFilter(String user){ + return authorizationDao.selectGlobalPermissions(user).contains(Permission.DASHBOARD_SHARING.key()); } String getLoggedLogin(UserSession userSession) { @@ -218,8 +218,8 @@ public class IssueFilterService implements ServerComponent { } } - private void verifyCurrentUserCanModifyFilter(DefaultIssueFilter issueFilter, UserSession userSession) { - if (!issueFilter.user().equals(userSession.login()) && !isAdmin(userSession)) { + private void verifyCurrentUserCanModifyFilter(DefaultIssueFilter issueFilter, String user) { + if (!issueFilter.user().equals(user) && !isAdmin(user)) { throw new ForbiddenException("User is not authorized to modify this filter"); } } @@ -230,19 +230,19 @@ public class IssueFilterService implements ServerComponent { } } - private void verifyCurrentUserCanChangeFilterOwnership(UserSession userSession) { - if (!isAdmin(userSession)) { + private void verifyCurrentUserCanChangeFilterOwnership(String user) { + if (!isAdmin(user)) { throw new ForbiddenException("User is not authorized to change the owner of this filter"); } } - 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 verifyCurrentUserCanShareFilter(DefaultIssueFilter issueFilter, String user) { + if (issueFilter.shared() && !canShareFilter(user)) { + throw new ForbiddenException("User cannot own this filter because of insufficient rights"); } } - private void validateFilter(final DefaultIssueFilter issueFilter, UserSession userSession) { + private void validateFilter(final DefaultIssueFilter issueFilter) { List userFilters = selectUserIssueFilters(issueFilter.user()); IssueFilterDto userFilterSameName = findFilterWithSameName(userFilters, issueFilter.name()); if (userFilterSameName != null && !userFilterSameName.getId().equals(issueFilter.id())) { @@ -254,7 +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); + verifyCurrentUserCanShareFilter(issueFilter, issueFilter.user()); } } @@ -321,8 +321,8 @@ public class IssueFilterService implements ServerComponent { })); } - private boolean isAdmin(UserSession userSession) { - return userSession.hasGlobalPermission(Permission.SYSTEM_ADMIN); + private boolean isAdmin(String user) { + return authorizationDao.selectGlobalPermissions(user).contains(Permission.SYSTEM_ADMIN.key()); } private IssueFilterResult createIssueFilterResult(IssueQueryResult issueQueryResult, IssueQuery issueQuery) { diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb index e0e8e519dad..379c90aa4bc 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb @@ -321,8 +321,14 @@ class MeasureFilter < ActiveRecord::Base count = MeasureFilter.count('id', :conditions => ['name=? and shared=? and (user_id is null or user_id<>?)', name, true, user_id]) end errors.add_to_base('Other users already share filters with the same name') if count>0 + + # Verify filter owner has sharing permission + if user && !user.has_role?(:shareDashboard) + errors.add(:user, "cannot own this filter because of insufficient rights") + end elsif system? errors.add_to_base("System filters can't be unshared") end 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 82849fe0371..f2a7c891e50 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 @@ -541,7 +541,7 @@ public class InternalRubyIssueServiceTest { @Test public void should_check_if_user_can_share_issue_filter(){ service.canUserShareIssueFilter(); - verify(issueFilterService).canShareFilter(any(UserSession.class)); + verify(issueFilterService).canShareFilter(anyString()); } @Test 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 16f5948ae1b..a2cc7bd783c 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 @@ -242,7 +242,7 @@ public class IssueFilterServiceTest { @Test public void should_have_permission_to_share_filter() { - UserSession userSession = MockUserSession.create().setLogin("john").setPermissions(Permission.DASHBOARD_SHARING); + when(authorizationDao.selectGlobalPermissions("john")).thenReturn(newArrayList(Permission.DASHBOARD_SHARING.key())); 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(true).setUser("john"), userSession); @@ -253,14 +253,14 @@ public class IssueFilterServiceTest { @Test public void should_not_share_filter_if_no_permission() { - UserSession userSession = MockUserSession.create().setLogin("john").setPermissions(); + when(authorizationDao.selectGlobalPermissions("john")).thenReturn(Collections.emptyList()); 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"); + assertThat(e).isInstanceOf(ForbiddenException.class).hasMessage("User cannot own this filter because of insufficient rights"); } verify(issueFilterDao, never()).update(any(IssueFilterDto.class)); } @@ -268,7 +268,7 @@ public class IssueFilterServiceTest { @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(authorizationDao.selectGlobalPermissions("john")).thenReturn(newArrayList(Permission.SYSTEM_ADMIN.key())); when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("Arthur Filter").setShared(true).setUserLogin("arthur")); try { @@ -308,17 +308,33 @@ public class IssueFilterServiceTest { } @Test - public void should_update_other_shared_filter_if_admin() { - UserSession userSession = MockUserSession.create().setLogin("john").setPermissions(Permission.SYSTEM_ADMIN, Permission.DASHBOARD_SHARING); + public void should_update_other_shared_filter_if_admin_and_if_filter_owner_has_sharing_permission() { + when(authorizationDao.selectGlobalPermissions("john")).thenReturn(newArrayList(Permission.SYSTEM_ADMIN.key())); + when(authorizationDao.selectGlobalPermissions("arthur")).thenReturn(newArrayList(Permission.DASHBOARD_SHARING.key())); 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); + DefaultIssueFilter result = service.update(new DefaultIssueFilter().setId(1L).setName("My New Filter").setDescription("New description").setShared(true).setUser("arthur"), userSession); assertThat(result.name()).isEqualTo("My New Filter"); assertThat(result.description()).isEqualTo("New description"); verify(issueFilterDao).update(any(IssueFilterDto.class)); } + @Test + public void should_not_update_other_shared_filter_if_admin_and_if_filter_owner_has_no_sharing_permission() { + when(authorizationDao.selectGlobalPermissions("john")).thenReturn(newArrayList(Permission.SYSTEM_ADMIN.key())); + when(authorizationDao.selectGlobalPermissions("arthur")).thenReturn(Collections.emptyList()); + when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Old Filter").setDescription("Old description").setUserLogin("arthur").setShared(true)); + + try { + service.update(new DefaultIssueFilter().setId(1L).setName("My New Filter").setDescription("New description").setShared(true).setUser("arthur"), userSession); + fail(); + } catch (Exception e) { + assertThat(e).isInstanceOf(ForbiddenException.class).hasMessage("User cannot own this filter because of insufficient rights"); + } + verify(issueFilterDao, never()).update(any(IssueFilterDto.class)); + } + @Test public void should_not_update_if_filter_not_found() { when(issueFilterDao.selectById(1L)).thenReturn(null); @@ -376,11 +392,12 @@ public class IssueFilterServiceTest { @Test public void should_change_shared_filter_ownership_when_admin() throws Exception { - String currentUser = "dave.loper"; 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); - UserSession userSession = MockUserSession.create().setLogin(currentUser).setPermissions(Permission.SYSTEM_ADMIN, Permission.DASHBOARD_SHARING); + // New owner should have sharing perm in order to own the filter + when(authorizationDao.selectGlobalPermissions("new.owner")).thenReturn(newArrayList(Permission.DASHBOARD_SHARING.key())); + when(authorizationDao.selectGlobalPermissions("john")).thenReturn(newArrayList(Permission.SYSTEM_ADMIN.key())); when(issueFilterDao.selectById(1L)).thenReturn(sharedFilter); when(issueFilterDao.selectSharedFilters()).thenReturn(Lists.newArrayList(sharedFilter)); @@ -445,7 +462,7 @@ public class IssueFilterServiceTest { @Test public void should_delete_shared_filter_if_user_is_admin() { - UserSession userSession = MockUserSession.create().setLogin("john").setPermissions(Permission.SYSTEM_ADMIN); + when(authorizationDao.selectGlobalPermissions("john")).thenReturn(newArrayList(Permission.SYSTEM_ADMIN.key())); when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Issues").setUserLogin("arthur").setShared(true)); service.delete(1L, userSession);