From b68b03846c34ea7dbd73b8228be5b7ffde46bcff Mon Sep 17 00:00:00 2001 From: sofyenne Date: Wed, 20 Nov 2024 15:17:59 +0100 Subject: [PATCH] feat: Disallow space members from editing or removing articles they did not author - EXO-75398 - Meeds-io/meeds#2603 --- .../news/service/impl/NewsServiceImpl.java | 29 +++++++++++++------ .../service/impl/NewsServiceImplTest.java | 26 ++++++++++------- 2 files changed, 35 insertions(+), 20 deletions(-) diff --git a/content-service/src/main/java/io/meeds/news/service/impl/NewsServiceImpl.java b/content-service/src/main/java/io/meeds/news/service/impl/NewsServiceImpl.java index d56b50b2d..5b17c088d 100644 --- a/content-service/src/main/java/io/meeds/news/service/impl/NewsServiceImpl.java +++ b/content-service/src/main/java/io/meeds/news/service/impl/NewsServiceImpl.java @@ -512,7 +512,7 @@ public News getNewsByIdAndLang(String newsId, throw new IllegalAccessException("User " + currentIdentity.getUserId() + " is not authorized to view News"); } news.setCanEdit(canEditNews(news, currentIdentity.getUserId())); - news.setCanDelete(canDeleteNews(currentIdentity, news.getAuthor(), news.getSpaceId())); + news.setCanDelete(canDeleteNews(currentIdentity, news)); news.setCanPublish(NewsUtils.canPublishNews(news.getSpaceId(), currentIdentity)); Space space = spaceService.getSpaceById(news.getSpaceId()); if (space != null) { @@ -578,7 +578,7 @@ public List getNews(NewsFilter filter, Identity currentIdentity) throws Ex } newsList.stream().filter(Objects::nonNull).forEach(news -> { news.setCanEdit(canEditNews(news, currentIdentity.getUserId())); - news.setCanDelete(canDeleteNews(currentIdentity, news.getAuthor(), news.getSpaceId())); + news.setCanDelete(canDeleteNews(currentIdentity, news)); news.setCanPublish(NewsUtils.canPublishNews(news.getSpaceId(), currentIdentity)); Space space = spaceService.getSpaceById(news.getSpaceId()); if (space != null) { @@ -817,8 +817,8 @@ public boolean canScheduleNews(Space space, Identity currentIdentity, News artic boolean isArticleAuthor = article.getAuthor() != null && article.getAuthor().equals(currentIdentity.getUserId()); boolean spaceMemberCanSchedule = isArticleAuthor && spaceService.isMember(space, currentIdentity.getUserId()); return spaceMemberCanSchedule || spaceService.isManager(space, currentIdentity.getUserId()) - || spaceService.isRedactor(space, currentIdentity.getUserId()) - || NewsUtils.canPublishNews(space.getId(), currentIdentity); + || spaceService.isRedactor(space, currentIdentity.getUserId()) + || NewsUtils.canPublishNews(space.getId(), currentIdentity); } /** @@ -1580,16 +1580,27 @@ private boolean canEditNews(News news, String authenticatedUser) { LOG.warn("Can't find user with id {} when checking access on news with id {}", authenticatedUser, news.getId()); return false; } - return NewsUtils.canPublishNews(news.getSpaceId(), authenticatedUserIdentity) - || spaceService.canRedactOnSpace(space, authenticatedUserIdentity); + boolean isSpaceMemberCanEdit = isArticleAuthor(news, authenticatedUser) && spaceService.isMember(spaceId, authenticatedUser); + return isSpaceMemberCanEdit || NewsUtils.canPublishNews(news.getSpaceId(), authenticatedUserIdentity) + || (spaceService.isManager(space, authenticatedUser) + || spaceService.isSuperManager(space, authenticatedUser) + || spaceService.isRedactor(space, authenticatedUser)); } - private boolean canDeleteNews(Identity currentIdentity, String posterId, String spaceId) { - Space space = spaceId == null ? null : spaceService.getSpaceById(spaceId); + private boolean isArticleAuthor(News article, String userName) { + return StringUtils.isNotEmpty(article.getAuthor()) && article.getAuthor().equals(userName); + } + + private boolean canDeleteNews(Identity currentIdentity, News news) { + Space space = news.getSpaceId() == null ? null : spaceService.getSpaceById(news.getSpaceId()); if (space == null) { return false; } - return spaceService.canRedactOnSpace(space, currentIdentity); + boolean isMemberCanDelete = isArticleAuthor(news, currentIdentity.getUserId()) + && spaceService.isMember(space.getId(), currentIdentity.getUserId()); + return isMemberCanDelete || (spaceService.isManager(space, currentIdentity.getUserId()) + || spaceService.isSuperManager(space, currentIdentity.getUserId()) + || spaceService.isRedactor(space, currentIdentity.getUserId())); } private boolean canViewSharedInSpaces(News news, String username) { diff --git a/content-service/src/test/java/io/meeds/news/service/impl/NewsServiceImplTest.java b/content-service/src/test/java/io/meeds/news/service/impl/NewsServiceImplTest.java index 04eb6f965..495d9508f 100644 --- a/content-service/src/test/java/io/meeds/news/service/impl/NewsServiceImplTest.java +++ b/content-service/src/test/java/io/meeds/news/service/impl/NewsServiceImplTest.java @@ -320,7 +320,7 @@ public void testUpdateDraftArticle() throws Exception { assertThrows(IllegalAccessException.class, () -> newsService.updateNews(news, "john", false, false, NewsUtils.NewsObjectType.DRAFT.name().toLowerCase(), CONTENT_AND_TITLE.name())); // Given - when(spaceService.canRedactOnSpace(space, identity)).thenReturn(true); + when(spaceService.isSuperManager(any(Space.class), anyString())).thenReturn(true); org.exoplatform.social.core.identity.model.Identity identity1 = mock(org.exoplatform.social.core.identity.model.Identity.class); when(identityManager.getOrCreateUserIdentity(anyString())).thenReturn(identity1); @@ -365,9 +365,13 @@ public void testDeleteDraftArticle() throws Exception { when(rootPage.getName()).thenReturn(NEWS_ARTICLES_ROOT_NOTE_PAGE_NAME); when(noteService.getDraftNoteById(anyString(), anyString())).thenReturn(draftPage); NEWS_UTILS.when(() -> NewsUtils.getUserIdentity(anyString())).thenReturn(identity); - when(spaceService.canRedactOnSpace(space, identity)).thenReturn(true); + + // + assertThrows(IllegalAccessException.class, () -> newsService.deleteNews(draftPage.getId(), identity, NewsUtils.NewsObjectType.DRAFT.name().toLowerCase())); // When + when(spaceService.isSuperManager(any(Space.class), anyString())).thenReturn(true); + newsService.deleteNews(draftPage.getId(), identity, NewsUtils.NewsObjectType.DRAFT.name().toLowerCase()); // Then @@ -616,8 +620,7 @@ public void testCreateDraftArticleForExistingPage() throws Exception { assertThrows(IllegalAccessException.class, () -> newsService.updateNews(news, "john", false, false, NewsUtils.NewsObjectType.DRAFT.name().toLowerCase(), CONTENT_AND_TITLE.name())); // Given - when(spaceService.canRedactOnSpace(space, identity)).thenReturn(true); - when(spaceService.isSuperManager(anyString())).thenReturn(true); + when(spaceService.isSuperManager(any(Space.class), anyString())).thenReturn(true); org.exoplatform.social.core.identity.model.Identity identity1 = mock(org.exoplatform.social.core.identity.model.Identity.class); when(identityManager.getOrCreateUserIdentity(anyString())).thenReturn(identity1); @@ -700,8 +703,7 @@ public void testUpdateDraftArticleForExistingPage() throws Exception { assertThrows(IllegalAccessException.class, () -> newsService.updateNews(news, "john", false, false, NewsUtils.NewsObjectType.DRAFT.name().toLowerCase(), CONTENT_AND_TITLE.name())); // Given - when(spaceService.canRedactOnSpace(space, identity)).thenReturn(true); - when(spaceService.isSuperManager(anyString())).thenReturn(true); + when(spaceService.isSuperManager(any(Space.class), anyString())).thenReturn(true); org.exoplatform.social.core.identity.model.Identity identity1 = mock(org.exoplatform.social.core.identity.model.Identity.class); when(identityManager.getOrCreateUserIdentity(anyString())).thenReturn(identity1); @@ -776,8 +778,7 @@ public void testUpdateNewsArticle() throws Exception { assertThrows(IllegalAccessException.class, () -> newsService.updateNews(news, "john", false, false, NewsUtils.NewsObjectType.DRAFT.name().toLowerCase(), CONTENT_AND_TITLE.name())); // Given - when(spaceService.canRedactOnSpace(space, identity)).thenReturn(true); - when(spaceService.isSuperManager(anyString())).thenReturn(true); + when(spaceService.isSuperManager(any(Space.class), anyString())).thenReturn(true); org.exoplatform.social.core.identity.model.Identity identity1 = mock(org.exoplatform.social.core.identity.model.Identity.class); when(identityManager.getOrCreateUserIdentity(anyString())).thenReturn(identity1); @@ -829,7 +830,11 @@ public void testDeleteNewsArticle() throws Exception { assertThrows(IllegalAccessException.class, () -> newsService.deleteNews(existingPage.getId(), identity, ARTICLE.name().toLowerCase())); // when - when(spaceService.canRedactOnSpace(space, identity)).thenReturn(true); + when(spaceService.isSuperManager(any(Space.class), anyString())).thenReturn(false); + when(spaceService.isRedactor(any(Space.class), anyString())).thenReturn(false); + when(spaceService.isManager(any(Space.class), anyString())).thenReturn(false); + when(spaceService.isMember(anyString(), anyString())).thenReturn(true); + when(noteService.deleteNote(existingPage.getWikiType(), existingPage.getWikiOwner(), existingPage.getName())).thenReturn(true); DraftPage draftPage = mock(DraftPage.class); NotePageProperties draftProperties = new NotePageProperties(); @@ -1046,8 +1051,7 @@ public void testAddNewsArticleTranslation() throws Exception { assertThrows(IllegalAccessException.class, () -> newsService.updateNews(news, "john", false, false, NewsUtils.NewsObjectType.DRAFT.name().toLowerCase(), CONTENT_AND_TITLE.name())); // Given - when(spaceService.canRedactOnSpace(space, identity)).thenReturn(true); - when(spaceService.isSuperManager(anyString())).thenReturn(true); + when(spaceService.isSuperManager(any(Space.class), anyString())).thenReturn(true); org.exoplatform.social.core.identity.model.Identity identity1 = mock(org.exoplatform.social.core.identity.model.Identity.class); when(identityManager.getOrCreateUserIdentity(anyString())).thenReturn(identity1);