Skip to content

Commit 5f5be6a

Browse files
boubakerclaude
andauthored
fix: The site layout editor refuses a draft it can't tell apart from another user's, and a deleted user's draft sites are removed EXO-91427 (#604)
**Symptom**: some draft site names also read as another user's draft: a site name holding `_draft_` or ending with `_draft`, or a username starting with `draft_`. On such a name, opening the site layout editor removed and re-created the draft by name, which could drop the other user's draft. Once portal refuses such a name (Meeds-io/portal#1345), the editor stopped on a generic error instead. A deleted user's draft sites stayed, and a user whose name fits a leftover name could open them. **Cause**: `SiteLayoutService#createDraftSite` builds `<type>_<name>_draft_<user>` and removes any site of that name before creating it, without checking that the user may edit that name. Nothing removed a user's draft sites when the user was deleted. **Fix**: - `createDraftSite` checks `LayoutAclService#canEditSite(PortalConfig, String)`, a new overload, on the draft it built in memory, before removing or creating anything. When the check fails it refuses with `IllegalArgumentException("layout.siteDraftNameConflict")`. `SiteLayoutRest` answers 400 with that message code, and the editor shows its text. Administrators pass the check. - `SiteLayoutService#deleteUserDraftSites` (`@ContainerTransactional`) removes every `DRAFT` site whose name ends with `_draft_<username>`, read in one unpaged listing. A removal that fails is logged and skipped. Two listeners call it off the deleting thread: `DraftSiteOwnerAccountDeletedListener` on social's `social.account.deleted` (`@Asynchronous`), and `DraftSiteOwnerUserDeletedListener` on `UserEventListener#postDelete` (`CompletableFuture.runAsync`). - Tests: - `SiteLayoutServiceTest`: the refusal leaves the existing draft untouched; the removal by suffix; a failed removal; a blank username; the container binding. - `LayoutAclServiceTest`: a site not stored yet. - `SiteLayoutRestTest`: the 400. - Both listener tests. **Known limits** (accepted): - an administrator passes the check; - resuming a draft through the user's co-editing revision shows the generic error, not the conflict message; - an owner whose draft name collides with another existing user's is refused that draft; - page and navigation-node drafts (`<name>_draft_<user>`) share the name ambiguity and are saved over by name; - the draft sites stay for users deleted before this change, and for a user deleted through social's `DELETE /v1/social/users/{id}`, which fires no event; - the removals run in one persistence context; - a co-editing revision can still name a removed draft; - removing a deleted user's draft sites also removes a draft whose name another user could own: while the deleted user existed, that name was refused to every non-administrator, and an administrator's draft of that name is removed too. Stacked on #581. Merge after it and after Meeds-io/portal#1345. Knowledge: Meeds-io/eng-standards#572, #522 This change is classified N1 (a new permission check on the draft site's write path): its approver must be an Archi/Dev who knows it is N1, not an approval on AI review alone, and author ≠ approver. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 8189268 commit 5f5be6a

12 files changed

Lines changed: 491 additions & 1 deletion

File tree

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/**
2+
* This file is part of the Meeds project (https://meeds.io/).
3+
*
4+
* Copyright (C) 2026 Meeds Association contact@meeds.io
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
13+
* Lesser General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU Lesser General Public License
16+
* along with this program; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
18+
*/
19+
package io.meeds.layout.listener;
20+
21+
import org.springframework.beans.factory.annotation.Autowired;
22+
import org.springframework.stereotype.Component;
23+
24+
import org.exoplatform.services.listener.Asynchronous;
25+
import org.exoplatform.services.listener.Event;
26+
import org.exoplatform.services.listener.ListenerBase;
27+
import org.exoplatform.services.listener.ListenerService;
28+
29+
import io.meeds.layout.service.SiteLayoutService;
30+
import io.meeds.social.security.service.AccountDeletionService;
31+
32+
import jakarta.annotation.PostConstruct;
33+
34+
/**
35+
* Removes the draft sites of an account the account deletion job deleted: the
36+
* job removes the user without the user event listeners.
37+
*/
38+
@Asynchronous
39+
@Component
40+
public class DraftSiteOwnerAccountDeletedListener implements ListenerBase<String, Object> {
41+
42+
@Autowired
43+
private ListenerService listenerService;
44+
45+
@Autowired
46+
private SiteLayoutService siteLayoutService;
47+
48+
@PostConstruct
49+
public void init() {
50+
listenerService.addListener(AccountDeletionService.ACCOUNT_DELETED_EVENT, this);
51+
}
52+
53+
@Override
54+
public void onEvent(Event<String, Object> event) {
55+
siteLayoutService.deleteUserDraftSites(event.getSource());
56+
}
57+
58+
}
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
/**
2+
* This file is part of the Meeds project (https://meeds.io/).
3+
*
4+
* Copyright (C) 2026 Meeds Association contact@meeds.io
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
13+
* Lesser General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU Lesser General Public License
16+
* along with this program; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
18+
*/
19+
package io.meeds.layout.listener;
20+
21+
import java.util.concurrent.CompletableFuture;
22+
23+
import org.springframework.beans.factory.annotation.Autowired;
24+
import org.springframework.stereotype.Component;
25+
26+
import org.exoplatform.services.log.ExoLogger;
27+
import org.exoplatform.services.log.Log;
28+
import org.exoplatform.services.organization.OrganizationService;
29+
import org.exoplatform.services.organization.User;
30+
import org.exoplatform.services.organization.UserEventListener;
31+
32+
import io.meeds.layout.service.SiteLayoutService;
33+
34+
import jakarta.annotation.PostConstruct;
35+
36+
/**
37+
* Removes the draft sites of a user deleted through the user handler with its
38+
* listeners, as an administrator's deletion is, outside the deleting request.
39+
*/
40+
@Component
41+
public class DraftSiteOwnerUserDeletedListener extends UserEventListener {
42+
43+
private static final Log LOG = ExoLogger.getLogger(DraftSiteOwnerUserDeletedListener.class);
44+
45+
@Autowired
46+
private OrganizationService organizationService;
47+
48+
@Autowired
49+
private SiteLayoutService siteLayoutService;
50+
51+
@PostConstruct
52+
public void init() {
53+
organizationService.getUserHandler().addUserEventListener(this);
54+
}
55+
56+
@Override
57+
public void postDelete(User user) {
58+
String username = user.getUserName();
59+
CompletableFuture.runAsync(() -> siteLayoutService.deleteUserDraftSites(username))
60+
.exceptionally(e -> {
61+
LOG.warn("Error while removing the draft sites of deleted user {}", username, e);
62+
return null;
63+
});
64+
}
65+
66+
}

‎layout-service/src/main/java/io/meeds/layout/rest/SiteLayoutRest.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,7 @@ public ResponseEntity<SiteEntity> createSite(
346346
@Operation(summary = "create a site", method = "POST", description = "This create a new site")
347347
@ApiResponses(value = {
348348
@ApiResponse(responseCode = "200", description = "Request fulfilled"),
349+
@ApiResponse(responseCode = "400", description = "The draft's name is also another user's draft"),
349350
@ApiResponse(responseCode = "403", description = "Forbidden"),
350351
@ApiResponse(responseCode = "404", description = "Not found"),
351352
@ApiResponse(responseCode = "409", description = "Site layout not readable"),
@@ -368,6 +369,8 @@ public ResponseEntity<SiteEntity> createDraftSite(
368369
throw new ResponseStatusException(HttpStatus.FORBIDDEN, e.getMessage());
369370
} catch (SiteLayoutUnresolvedException e) {
370371
throw new ResponseStatusException(HttpStatus.CONFLICT, e.getMessage());
372+
} catch (IllegalArgumentException e) {
373+
throw new ResponseStatusException(HttpStatus.BAD_REQUEST, e.getMessage());
371374
}
372375
}
373376

‎layout-service/src/main/java/io/meeds/layout/service/LayoutAclService.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,10 @@ public boolean canEditSite(SiteKey siteKey, String username) {
5858
if (portalConfig == null) {
5959
return false;
6060
}
61+
return canEditSite(portalConfig, username);
62+
}
63+
64+
public boolean canEditSite(PortalConfig portalConfig, String username) {
6165
return userAcl.hasEditPermission(portalConfig, userAcl.getUserIdentity(username));
6266
}
6367

‎layout-service/src/main/java/io/meeds/layout/service/SiteLayoutService.java‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import java.util.Collections;
2222
import java.util.HashMap;
23+
import java.util.List;
2324
import java.util.Locale;
2425
import java.util.Map;
2526
import java.util.Set;
@@ -38,6 +39,7 @@
3839
import org.exoplatform.portal.config.model.ModelObject;
3940
import org.exoplatform.portal.config.model.Page;
4041
import org.exoplatform.portal.config.model.PortalConfig;
42+
import org.exoplatform.portal.mop.SiteFilter;
4143
import org.exoplatform.portal.mop.SiteKey;
4244
import org.exoplatform.portal.mop.SiteType;
4345
import org.exoplatform.portal.mop.importer.ImportMode;
@@ -53,6 +55,7 @@
5355
import org.exoplatform.services.resources.LocaleConfigService;
5456
import org.exoplatform.services.resources.LocaleContextInfo;
5557

58+
import io.meeds.common.ContainerTransactional;
5659
import io.meeds.layout.util.LayoutStyleValidator;
5760

5861
import lombok.SneakyThrows;
@@ -66,6 +69,10 @@ public class SiteLayoutService {
6669

6770
private static final String SITE_DOESNT_EXIST_MSG = "Site %s doesn't exist";
6871

72+
private static final String DRAFT_NAME_CONFLICT = "layout.siteDraftNameConflict";
73+
74+
private static final String DRAFT_OWNER_SEPARATOR = "_draft_";
75+
6976
private static final Log LOG = ExoLogger.getLogger(SiteLayoutService.class);
7077

7178
@Autowired
@@ -174,11 +181,16 @@ public SiteKey createDraftSite(SiteKey siteKey, String username) throws ObjectNo
174181
PortalConfig site = getSite(siteKey, username);
175182
SiteLayoutUnresolvedException.checkResolved(site);
176183

177-
String clonedSiteName = site.getType() + "_" + site.getName() + "_draft_" + username;
184+
String clonedSiteName = site.getType() + "_" + site.getName() + DRAFT_OWNER_SEPARATOR + username;
178185

179186
PortalConfig draftSite = site.clone();
180187
draftSite.setType(PortalConfig.DRAFT);
181188
draftSite.setName(clonedSiteName);
189+
if (!aclService.canEditSite(draftSite, username)) {
190+
// The name could also be another existing user's draft of another site:
191+
// the user couldn't open it, and replacing it could drop that user's draft
192+
throw new IllegalArgumentException(DRAFT_NAME_CONFLICT);
193+
}
182194
LayoutModel siteLayoutModel = new LayoutModel(site.getPortalLayout(), portletInstanceService, new PortletInstanceContext(false, null));
183195
siteLayoutModel.resetStorage();
184196
draftSite.setPortalLayout(siteLayoutModel.toSite().getPortalLayout());
@@ -191,6 +203,20 @@ public SiteKey createDraftSite(SiteKey siteKey, String username) throws ObjectNo
191203
return draftSiteKey;
192204
}
193205

206+
/**
207+
* Removes the draft sites of a deleted user: every draft site whose name ends
208+
* with {@code _draft_<username>}, a name another user could also have
209+
* produced included. A non-administrator couldn't open such a draft while the
210+
* deleted user existed and might afterwards; an administrator's draft of that
211+
* name is removed too.
212+
*
213+
* @param username deleted user
214+
*/
215+
@ContainerTransactional
216+
public void deleteUserDraftSites(String username) {
217+
removeUserDraftSites(username);
218+
}
219+
194220
public void updateSite(SiteUpdateModel updateModel, String username) throws IllegalAccessException,
195221
ObjectNotFoundException {
196222
SiteKey siteKey = new SiteKey(updateModel.getSiteType(), updateModel.getSiteName());
@@ -415,4 +441,29 @@ private void broadcastRestoredPages(SiteKey siteKey,
415441
}
416442
}
417443

444+
void removeUserDraftSites(String username) {
445+
if (StringUtils.isBlank(username)) {
446+
return;
447+
}
448+
String draftSiteNameSuffix = DRAFT_OWNER_SEPARATOR + username;
449+
// Sites are sorted by label, which a draft shares with the site it drafts:
450+
// pages of that order could skip a draft, so all draft sites are read at once
451+
SiteFilter siteFilter = new SiteFilter();
452+
siteFilter.setSiteType(SiteType.DRAFT);
453+
List<PortalConfig> userDraftSites = layoutService.getSites(siteFilter)
454+
.stream()
455+
.filter(draftSite -> StringUtils.endsWith(draftSite.getName(),
456+
draftSiteNameSuffix))
457+
.toList();
458+
userDraftSites.forEach(this::removeDraftSite);
459+
}
460+
461+
private void removeDraftSite(PortalConfig draftSite) {
462+
try {
463+
layoutService.remove(draftSite);
464+
} catch (Exception e) {
465+
LOG.warn("Error while removing the draft site {} of a deleted user", draftSite.getName(), e);
466+
}
467+
}
468+
418469
}
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
/**
2+
* This file is part of the Meeds project (https://meeds.io/).
3+
*
4+
* Copyright (C) 2026 Meeds Association contact@meeds.io
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
13+
* Lesser General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU Lesser General Public License
16+
* along with this program; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
18+
*/
19+
package io.meeds.layout.listener;
20+
21+
import static org.junit.jupiter.api.Assertions.assertTrue;
22+
import static org.mockito.Mockito.verify;
23+
24+
import org.junit.jupiter.api.Test;
25+
import org.junit.jupiter.api.extension.ExtendWith;
26+
import org.mockito.InjectMocks;
27+
import org.mockito.Mock;
28+
import org.mockito.junit.jupiter.MockitoExtension;
29+
30+
import org.exoplatform.services.listener.Asynchronous;
31+
import org.exoplatform.services.listener.Event;
32+
import org.exoplatform.services.listener.ListenerService;
33+
34+
import io.meeds.layout.service.SiteLayoutService;
35+
import io.meeds.social.security.service.AccountDeletionService;
36+
37+
import jakarta.annotation.PostConstruct;
38+
import lombok.SneakyThrows;
39+
40+
@ExtendWith(MockitoExtension.class)
41+
public class DraftSiteOwnerAccountDeletedListenerTest {
42+
43+
@Mock
44+
private ListenerService listenerService;
45+
46+
@Mock
47+
private SiteLayoutService siteLayoutService;
48+
49+
@InjectMocks
50+
private DraftSiteOwnerAccountDeletedListener listener;
51+
52+
/**
53+
* Spring calls init: the annotation-removed mutant passes the call made by
54+
* hand.
55+
*/
56+
@Test
57+
@SneakyThrows
58+
public void init() {
59+
listener.init();
60+
61+
verify(listenerService).addListener(AccountDeletionService.ACCOUNT_DELETED_EVENT, listener);
62+
assertTrue(DraftSiteOwnerAccountDeletedListener.class.getMethod("init").isAnnotationPresent(PostConstruct.class));
63+
}
64+
65+
/**
66+
* The listener service runs a listener outside the broadcasting thread only
67+
* when its class carries the annotation: the annotation-removed mutant passes
68+
* every other test.
69+
*/
70+
@Test
71+
public void isAsynchronous() {
72+
assertTrue(DraftSiteOwnerAccountDeletedListener.class.isAnnotationPresent(Asynchronous.class));
73+
}
74+
75+
@Test
76+
public void onEvent() {
77+
listener.onEvent(new Event<>(AccountDeletionService.ACCOUNT_DELETED_EVENT, "john", null));
78+
79+
verify(siteLayoutService).deleteUserDraftSites("john");
80+
}
81+
82+
}

0 commit comments

Comments
 (0)