From cd1ac7ee796681857511be200474d81f84eeb7b1 Mon Sep 17 00:00:00 2001 From: Sofien Haj Chedhli Date: Thu, 21 Nov 2024 10:13:23 +0100 Subject: [PATCH] feat: Disallow space members from editing or removing articles they did not author - EXO-75398 - Meeds-io/meeds#2603 (#315) Prior to this change, editing and deleting an article were based on the "can redact on space" API, which allowed members to redact in spaces that did not have a designated redactor. This logic caused an issue by allowing members to edit and delete articles they did not own. To resolve this issue, we kept the article creation logic based on the "can redact" API but updated the logic for updating and deleting articles. Now, permissions for editing and deleting articles are handled separately, preventing users from updating or deleting articles they do not own, thus fixing the issue --- .../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);