Skip to content

Commit

Permalink
feat: Disallow space members from editing or removing articles they d…
Browse files Browse the repository at this point in the history 
…id 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
  • Loading branch information
@sofyenne
sofyenne authored Nov 21, 2024
1 parent ca4349c commit cd1ac7e
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 20 deletions.
29 changes: 20 additions & 9 deletions content-service/src/main/java/io/meeds/news/service/impl/NewsServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -578,7 +578,7 @@ public List<News> 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) {
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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) {
Expand Down
26 changes: 15 additions & 11 deletions content-service/src/test/java/io/meeds/news/service/impl/NewsServiceImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit cd1ac7e

Please sign in to comment.