Make shared-file collaborative writes upgrade-safe and opt-in

2026-09-03 05:10:16 +03:00 · 2026-08-13 10:33:14 +01:00
parent de65fda91b
commit 6854b46568
18 changed files with 614 additions and 31 deletions
@@ -48,7 +48,7 @@ The File Sharing feature enables users to store files server-side and share them

 | Role | Can Read | Can Write |
 |------|----------|-----------|
-| `EDITOR` | ✅ | ✅ |
+| `EDITOR` | ✅ | ✅ (when write was granted, see below) |
 | `COMMENTER` | ✅ | ❌ |
 | `VIEWER` | ✅ | ❌ |

@@ -60,9 +60,24 @@ Owners always have full access regardless of role.
 `EDITOR` user-share can `PUT /api/v1/storage/files/{fileId}`, and any
 authenticated holder of an `EDITOR` share link can
 `PUT /api/v1/storage/share-links/{token}`. Non-owner writes require
-`sharing.enabled`. Non-owner updates replace only the main file content -
-history bundle and audit log parts are accepted but only the owner's client
-sends them, so the owner's audit trail is not clobbered by collaborators.
+`sharing.enabled`.
+
+#### Write Is Opt-In Per Share
+
+The `EDITOR` role alone does not grant write. `file_shares.write_enabled` records
+the grant, and it is stamped `true` only when the owner creates or updates a share
+with the `EDITOR` role. Shares that predate collaborative editing have the column
+null and stay read-only after an upgrade — the owner must re-grant editor access to
+make one writable. `StoredFileResponse.canEdit` and `ShareLinkMetadataResponse.canEdit`
+expose the server's decision so the UI does not offer a save that would be rejected.
+
+Non-owner updates replace only the main file content. The version history bundle and
+the audit log belong to the owner, so a request from a non-owner that carries a
+`historyBundle` or `auditLog` part is rejected with `403` rather than overwriting
+(and deleting) the owner's archive.
+
+A superseded blob is deleted only after the transaction commits, so a failed or
+rolled-back update can never leave the file without its bytes.

 ### Optimistic Concurrency (Collaboration)

@@ -382,7 +397,8 @@ storage:
 ### Access Control
 - All endpoints require authentication — there is no anonymous access
 - Owner-only operations enforced in service layer (not just controller)
- `requireReadAccess` checked on every download; `requireEditorAccess` checked on every non-owner content update
+- `requireReadAccess` checked on every download; every non-owner content update requires an `EDITOR` share with `write_enabled` set
+- Replacing the version history or audit log is owner-only

 ### Share Link Security
 - Tokens are UUIDs (random, not guessable)
@@ -273,6 +273,7 @@ public class FileStorageController {
                        share.getAccessRole() != null
                                ? share.getAccessRole().name().toLowerCase(Locale.ROOT)
                                : null)
+                .canEdit(ownedByCurrentUser || fileStorageService.allowsWrite(share))
                .version(file.contentVersionOrZero())
                .createdAt(share.getCreatedAt())
                .expiresAt(share.getExpiresAt())
@@ -0,0 +1,40 @@
+package stirling.software.proprietary.storage.converter;
+
+import java.util.Locale;
+
+import jakarta.persistence.AttributeConverter;
+import jakarta.persistence.Converter;
+
+import lombok.extern.slf4j.Slf4j;
+
+import stirling.software.proprietary.storage.model.FileShareAccessType;
+
+/**
+ * Persists the access type as plain text. {@code @Enumerated(STRING)} makes Hibernate emit a native
+ * enum column on H2/MySQL, which {@code ddl-auto=update} never widens, so every new constant would
+ * be unwritable on existing installs.
+ */
+@Converter
+@Slf4j
+public class FileShareAccessTypeConverter
+        implements AttributeConverter<FileShareAccessType, String> {
+
+    @Override
+    public String convertToDatabaseColumn(FileShareAccessType attribute) {
+        return attribute != null ? attribute.name() : null;
+    }
+
+    @Override
+    public FileShareAccessType convertToEntityAttribute(String dbData) {
+        if (dbData == null || dbData.isBlank()) {
+            return null;
+        }
+        try {
+            return FileShareAccessType.valueOf(dbData.trim().toUpperCase(Locale.ROOT));
+        } catch (IllegalArgumentException ex) {
+            // A row written by a newer version must not break the reader.
+            log.warn("Unknown file share access type {} in database", dbData);
+            return null;
+        }
+    }
+}
@@ -68,6 +68,11 @@ public class FileShare implements Serializable {
    @Column(name = "access_role")
    private ShareAccessRole accessRole;

+    // Opt-in collaborative write. Null on shares that predate it, so upgrades stay read-only
+    // until the owner re-grants editor access.
+    @Column(name = "write_enabled")
+    private Boolean writeEnabled;
+
    @Column(name = "expires_at")
    private LocalDateTime expiresAt;

@@ -6,9 +6,8 @@ import java.time.LocalDateTime;
 import org.hibernate.annotations.CreationTimestamp;

 import jakarta.persistence.Column;
+import jakarta.persistence.Convert;
 import jakarta.persistence.Entity;
-import jakarta.persistence.EnumType;
-import jakarta.persistence.Enumerated;
 import jakarta.persistence.FetchType;
 import jakarta.persistence.GeneratedValue;
 import jakarta.persistence.GenerationType;
@@ -23,6 +22,7 @@ import lombok.NoArgsConstructor;
 import lombok.Setter;

 import stirling.software.proprietary.security.model.User;
+import stirling.software.proprietary.storage.converter.FileShareAccessTypeConverter;

@Entity
@Table(
@@ -54,8 +54,9 @@ public class FileShareAccess implements Serializable {
    @JoinColumn(name = "user_id", nullable = false)
    private User user;

-    @Enumerated(EnumType.STRING)
-    @Column(name = "access_type", nullable = false)
+    // Plain text, not @Enumerated: a native enum column cannot gain values on ddl-auto=update.
+    @Convert(converter = FileShareAccessTypeConverter.class)
+    @Column(name = "access_type", nullable = false, length = 32)
    private FileShareAccessType accessType;

    @CreationTimestamp
@@ -15,6 +15,9 @@ public class ShareLinkMetadataResponse {
    private final boolean ownedByCurrentUser;
    private final String accessRole;

+    // Whether the viewer may write back. Editor role alone is not enough on legacy shares.
+    private final boolean canEdit;
+
    // Content revision for optimistic concurrency; clients echo it back on update.
    private final Long version;
    private final LocalDateTime createdAt;
@@ -18,6 +18,9 @@ public class StoredFileResponse {
    private final boolean ownedByCurrentUser;
    private final String accessRole;

+    // Whether this user may write back. Editor role alone is not enough on legacy shares.
+    private final boolean canEdit;
+
    // Content revision for optimistic concurrency; clients echo it back on update.
    private final long version;
    private final LocalDateTime createdAt;
@@ -20,6 +20,8 @@ import org.springframework.security.core.Authentication;
 import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.stereotype.Service;
 import org.springframework.transaction.annotation.Transactional;
+import org.springframework.transaction.support.TransactionSynchronization;
+import org.springframework.transaction.support.TransactionSynchronizationManager;
 import org.springframework.web.multipart.MultipartFile;
 import org.springframework.web.server.ResponseStatusException;

@@ -192,16 +194,14 @@ public class FileStorageService {
            MultipartFile auditLog,
            Long expectedVersion) {
        ensureStorageEnabled();
-        if (!isOwner(existing, actor)) {
+        boolean owner = isOwner(existing, actor);
+        if (!owner) {
            // Collaborative write-back: shared EDITORs may replace content when sharing is on.
            ensureSharingEnabled();
-            ShareAccessRole role = resolveUserShareRole(existing, actor);
-            if (role != ShareAccessRole.EDITOR) {
-                throw new ResponseStatusException(
-                        HttpStatus.FORBIDDEN, "Editor access is required to update this file");
-            }
+            requireCollaborativeWriteAccess(
+                    fileShareRepository.findByFileAndSharedWithUser(existing, actor).orElse(null));
        }
-        return replaceFileContent(existing, file, historyBundle, auditLog, expectedVersion);
+        return replaceFileContent(existing, file, historyBundle, auditLog, expectedVersion, owner);
    }

    /** Content replace via an EDITOR share link; the actor is not the owner. */
@@ -213,8 +213,37 @@ public class FileStorageService {
            Long expectedVersion) {
        ensureStorageEnabled();
        ensureShareLinksEnabled();
-        requireEditorAccess(share);
-        return replaceFileContent(share.getFile(), file, historyBundle, auditLog, expectedVersion);
+        requireCollaborativeWriteAccess(share);
+        return replaceFileContent(
+                share.getFile(), file, historyBundle, auditLog, expectedVersion, false);
+    }
+
+    // Write is opt-in per share: the editor role alone is not enough, because every share that
+    // predates collaborative editing would otherwise become writable on upgrade.
+    private void requireCollaborativeWriteAccess(FileShare share) {
+        if (share == null || resolveShareRole(share) != ShareAccessRole.EDITOR) {
+            throw new ResponseStatusException(
+                    HttpStatus.FORBIDDEN, "Editor access is required to update this file");
+        }
+        if (!Boolean.TRUE.equals(share.getWriteEnabled())) {
+            throw new ResponseStatusException(
+                    HttpStatus.FORBIDDEN,
+                    "This share is read-only. The owner must re-grant editor access to enable"
+                            + " editing.");
+        }
+    }
+
+    public boolean allowsWrite(FileShare share) {
+        return share != null
+                && Boolean.TRUE.equals(share.getWriteEnabled())
+                && resolveShareRole(share) == ShareAccessRole.EDITOR;
+    }
+
+    private FileShare userShare(StoredFile file, User user) {
+        if (file == null || user == null) {
+            return null;
+        }
+        return fileShareRepository.findByFileAndSharedWithUser(file, user).orElse(null);
    }

    // Runs inside the transaction so lazy owner/share fields are still attached
@@ -243,6 +272,7 @@ public class FileStorageService {
                        share.getAccessRole() != null
                                ? share.getAccessRole().name().toLowerCase(Locale.ROOT)
                                : null)
+                .canEdit(ownedByCurrentUser || allowsWrite(share))
                .version(updated.contentVersionOrZero())
                .createdAt(share.getCreatedAt())
                .expiresAt(share.getExpiresAt())
@@ -254,8 +284,16 @@ public class FileStorageService {
            MultipartFile file,
            MultipartFile historyBundle,
            MultipartFile auditLog,
-            Long expectedVersion) {
+            Long expectedVersion,
+            boolean ownerWrite) {
        validateMainUpload(file);
+        // The version history and audit bundle are the owner's record of the file; a collaborator
+        // replacing them would destroy evidence they never owned.
+        if (!ownerWrite && (isValidUpload(historyBundle) || isValidUpload(auditLog))) {
+            throw new ResponseStatusException(
+                    HttpStatus.FORBIDDEN,
+                    "Only the owner can replace the version history or audit log");
+        }
        // Quotas and blob attribution always follow the file owner, not the acting editor.
        User owner = existing.getOwner();

@@ -301,12 +339,12 @@ public class FileStorageService {
                cleanupStoredObject(auditObject);
                throw saveError;
            }
-            cleanupStoredKey(oldStorageKey);
+            deleteAfterCommit(oldStorageKey);
            if (historyObject != null) {
-                cleanupStoredKey(oldHistoryKey);
+                deleteAfterCommit(oldHistoryKey);
            }
            if (auditObject != null) {
-                cleanupStoredKey(oldAuditKey);
+                deleteAfterCommit(oldAuditKey);
            }

            return updated;
@@ -459,19 +497,19 @@ public class FileStorageService {

    public List<StoredFileResponse> listAccessibleFileResponses(User user) {
        List<StoredFile> files = listAccessibleFiles(user);
-        Map<Long, ShareAccessRole> roleByFileId = new HashMap<>();
+        Map<Long, FileShare> shareByFileId = new HashMap<>();
        if (!files.isEmpty()) {
            List<FileShare> shares = fileShareRepository.findBySharedWithUserAndFileIn(user, files);
            for (FileShare share : shares) {
                StoredFile sharedFile = share.getFile();
                if (sharedFile != null && sharedFile.getId() != null) {
-                    roleByFileId.put(sharedFile.getId(), resolveShareRole(share));
+                    shareByFileId.put(sharedFile.getId(), share);
                }
            }
        }
        return files.stream()
                .sorted(Comparator.comparing(StoredFile::getCreatedAt).reversed())
-                .map(file -> buildResponse(file, user, roleByFileId.get(file.getId())))
+                .map(file -> buildResponse(file, user, shareByFileId.get(file.getId())))
                .toList();
    }

@@ -492,16 +530,24 @@ public class FileStorageService {
        return buildResponse(file, currentUser, null);
    }

+    // knownShare is the caller's already-loaded share for this user, so the list path stays a
+    // single bulk query instead of one lookup per file.
    private StoredFileResponse buildResponse(
-            StoredFile file, User currentUser, ShareAccessRole accessRoleOverride) {
+            StoredFile file, User currentUser, FileShare knownShare) {
        boolean ownedByCurrentUser =
                file.getOwner() != null
                        && Objects.equals(file.getOwner().getId(), currentUser.getId());
+        FileShare currentUserShare =
+                ownedByCurrentUser
+                        ? null
+                        : Optional.ofNullable(knownShare)
+                                .orElseGet(() -> userShare(file, currentUser));
        String accessRole =
                ownedByCurrentUser
                        ? ShareAccessRole.EDITOR.name().toLowerCase(Locale.ROOT)
-                        : Optional.ofNullable(accessRoleOverride)
-                                .orElseGet(() -> resolveUserShareRole(file, currentUser))
+                        : (currentUserShare != null
+                                        ? resolveShareRole(currentUserShare)
+                                        : ShareAccessRole.VIEWER)
                                .name()
                                .toLowerCase(Locale.ROOT);
        List<String> sharedWithUsers =
@@ -561,6 +607,7 @@ public class FileStorageService {
                .owner(file.getOwner() != null ? file.getOwner().getUsername() : null)
                .ownedByCurrentUser(ownedByCurrentUser)
                .accessRole(accessRole)
+                .canEdit(ownedByCurrentUser || allowsWrite(currentUserShare))
                .version(file.contentVersionOrZero())
                .createdAt(file.getCreatedAt())
                .updatedAt(file.getUpdatedAt())
@@ -635,7 +682,7 @@ public class FileStorageService {
        }
        storedFileRepository.delete(file);
        for (String storageKey : storageKeys) {
-            cleanupStoredKey(storageKey);
+            deleteAfterCommit(storageKey);
        }
    }

@@ -664,6 +711,7 @@ public class FileStorageService {
                            .map(
                                    existingShare -> {
                                        existingShare.setAccessRole(role);
+                                        existingShare.setWriteEnabled(grantsWrite(role));
                                        return fileShareRepository.save(existingShare);
                                    })
                            .orElseGet(
@@ -672,6 +720,7 @@ public class FileStorageService {
                                        newShare.setFile(file);
                                        newShare.setSharedWithUser(targetUser);
                                        newShare.setAccessRole(role);
+                                        newShare.setWriteEnabled(grantsWrite(role));
                                        return fileShareRepository.save(newShare);
                                    });

@@ -754,10 +803,16 @@ public class FileStorageService {
        share.setFile(file);
        share.setShareToken(UUID.randomUUID().toString());
        share.setAccessRole(role);
+        share.setWriteEnabled(grantsWrite(role));
        share.setExpiresAt(resolveShareLinkExpiration());
        return fileShareRepository.save(share);
    }

+    // Stamped at grant time so the decision is recorded on the share, not inferred later.
+    private boolean grantsWrite(ShareAccessRole role) {
+        return role == ShareAccessRole.EDITOR;
+    }
+
    public void revokeShareLink(User owner, StoredFile file, String token) {
        ensureStorageEnabled();
        if (!isOwner(file, owner)) {
@@ -885,7 +940,10 @@ public class FileStorageService {
                                                access.getUser() != null
                                                        ? access.getUser().getUsername()
                                                        : null)
-                                        .accessType(access.getAccessType().name())
+                                        .accessType(
+                                                access.getAccessType() != null
+                                                        ? access.getAccessType().name()
+                                                        : null)
                                        .accessedAt(access.getAccessedAt())
                                        .build())
                .toList();
@@ -938,6 +996,7 @@ public class FileStorageService {
                                                            .name()
                                                            .toLowerCase(Locale.ROOT)
                                                    : null)
+                                    .canEdit(allowsWrite(share))
                                    .version(file != null ? file.contentVersionOrZero() : null)
                                    .createdAt(share != null ? share.getCreatedAt() : null)
                                    .expiresAt(share != null ? share.getExpiresAt() : null)
@@ -1144,6 +1203,25 @@ public class FileStorageService {
        cleanupStoredKey(storedObject.getStorageKey());
    }

+    // Superseded blobs must outlive the transaction: a rollback restores the row that still
+    // points at them, so deleting inline would destroy the only copy of the file.
+    private void deleteAfterCommit(String storageKey) {
+        if (storageKey == null || storageKey.isBlank()) {
+            return;
+        }
+        if (!TransactionSynchronizationManager.isSynchronizationActive()) {
+            cleanupStoredKey(storageKey);
+            return;
+        }
+        TransactionSynchronizationManager.registerSynchronization(
+                new TransactionSynchronization() {
+                    @Override
+                    public void afterCommit() {
+                        cleanupStoredKey(storageKey);
+                    }
+                });
+    }
+
    private void cleanupStoredKey(String storageKey) {
        if (storageKey == null || storageKey.isBlank()) {
            return;
@@ -0,0 +1,116 @@
+package stirling.software.proprietary.storage.model;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.UUID;
+
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.SpringBootConfiguration;
+import org.springframework.boot.autoconfigure.AutoConfigurationPackage;
+import org.springframework.boot.data.jpa.test.autoconfigure.DataJpaTest;
+import org.springframework.boot.jdbc.test.autoconfigure.AutoConfigureTestDatabase;
+import org.springframework.test.annotation.DirtiesContext;
+import org.springframework.test.context.DynamicPropertyRegistry;
+import org.springframework.test.context.DynamicPropertySource;
+
+import jakarta.persistence.EntityManager;
+
+import stirling.software.proprietary.model.Team;
+import stirling.software.proprietary.security.model.User;
+
+/**
+ * Runs against a real v2.10.0 user database, upgraded in place by ddl-auto=update, because the
+ * mocked service tests never touch a column definition. The v2.10.0 schema pins access_type to the
+ * enum values that existed then, so a new constant is unwritable on every upgraded install unless
+ * the mapping is plain text.
+ */
+@DataJpaTest
+@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)
+@DirtiesContext
+class FileShareAccessTypeLegacySchemaDbTest {
+
+    private static Path databaseDir;
+
+    @Autowired private EntityManager entityManager;
+
+    @DynamicPropertySource
+    static void legacyDatabase(DynamicPropertyRegistry registry) throws Exception {
+        databaseDir = Files.createTempDirectory("legacy-share-schema");
+        Path database = databaseDir.resolve("legacy.mv.db");
+        try (InputStream fixture =
+                FileShareAccessTypeLegacySchemaDbTest.class.getResourceAsStream(
+                        "/db-migration-fixtures/stirling-pdf-v2.10.0.mv.db")) {
+            Files.copy(fixture, database);
+        }
+        String path = database.toString().replace('\\', '/');
+        String base = path.substring(0, path.length() - ".mv.db".length());
+        registry.add(
+                "spring.datasource.url",
+                () -> "jdbc:h2:file:" + base + ";DB_CLOSE_DELAY=-1;MODE=PostgreSQL");
+        registry.add("spring.datasource.username", () -> "sa");
+        registry.add("spring.datasource.password", () -> "");
+        registry.add("spring.datasource.driver-class-name", () -> "org.h2.Driver");
+        // Production upgrade strategy; it adds columns but never rewrites an existing one.
+        registry.add("spring.jpa.hibernate.ddl-auto", () -> "update");
+    }
+
+    @Test
+    void editAccessTypeIsWritableAgainstAPreEditSchema() {
+        Team team = new Team();
+        team.setName("team-" + UUID.randomUUID());
+        entityManager.persist(team);
+
+        User owner = new User();
+        owner.setUsername("owner-" + UUID.randomUUID());
+        owner.setPassword("x");
+        owner.setTeam(team);
+        entityManager.persist(owner);
+
+        StoredFile file = new StoredFile();
+        file.setOwner(owner);
+        file.setOriginalFilename("doc.pdf");
+        file.setContentType("application/pdf");
+        file.setSizeBytes(1);
+        file.setStorageKey("k-" + UUID.randomUUID());
+        entityManager.persist(file);
+
+        FileShare share = new FileShare();
+        share.setFile(file);
+        share.setSharedWithUser(owner);
+        share.setShareToken(UUID.randomUUID().toString());
+        share.setAccessRole(ShareAccessRole.EDITOR);
+        entityManager.persist(share);
+
+        FileShareAccess access = new FileShareAccess();
+        access.setFileShare(share);
+        access.setUser(owner);
+        access.setAccessType(FileShareAccessType.EDIT);
+        entityManager.persist(access);
+        entityManager.flush();
+        entityManager.clear();
+
+        FileShareAccess reloaded = entityManager.find(FileShareAccess.class, access.getId());
+        assertThat(reloaded.getAccessType()).isEqualTo(FileShareAccessType.EDIT);
+    }
+
+    @Test
+    void accessTypeColumnIsNoLongerANativeEnum() {
+        Object dataType =
+                entityManager
+                        .createNativeQuery(
+                                "SELECT DATA_TYPE FROM INFORMATION_SCHEMA.COLUMNS"
+                                        + " WHERE TABLE_NAME = 'FILE_SHARE_ACCESSES'"
+                                        + " AND COLUMN_NAME = 'ACCESS_TYPE'")
+                        .getSingleResult();
+
+        assertThat(String.valueOf(dataType)).isNotEqualTo("ENUM");
+    }
+
+    @SpringBootConfiguration
+    @AutoConfigurationPackage(basePackages = "stirling.software.proprietary")
+    static class TestApp {}
+}
@@ -142,6 +142,39 @@ class StoredFileMigrationQueriesDbTest {
                .containsExactly(file.getId());
    }

+    @Test
+    void bumpContentVersionIfMatches_rejectsStaleExpectationAndIncrementsOnMatch() {
+        StoredFile file = persistFile("k-version", null);
+        entityManager.clear();
+
+        // Legacy rows have no version at all; 0 is the expectation that matches them.
+        assertThat(repository.bumpContentVersionIfMatches(file.getId(), 3L)).isZero();
+        assertThat(repository.bumpContentVersionIfMatches(file.getId(), 0L)).isEqualTo(1);
+        entityManager.clear();
+
+        assertThat(repository.findById(file.getId()).orElseThrow().getContentVersion())
+                .isEqualTo(1L);
+        // The expectation that just won is now stale, so a replayed save must lose.
+        assertThat(repository.bumpContentVersionIfMatches(file.getId(), 0L)).isZero();
+        assertThat(repository.bumpContentVersionIfMatches(file.getId(), 1L)).isEqualTo(1);
+        entityManager.clear();
+
+        assertThat(repository.findById(file.getId()).orElseThrow().getContentVersion())
+                .isEqualTo(2L);
+    }
+
+    @Test
+    void bumpContentVersion_incrementsLegacyNullVersion() {
+        StoredFile file = persistFile("k-unconditional", null);
+        entityManager.clear();
+
+        assertThat(repository.bumpContentVersion(file.getId())).isEqualTo(1);
+        entityManager.clear();
+
+        assertThat(repository.findById(file.getId()).orElseThrow().getContentVersion())
+                .isEqualTo(1L);
+    }
+
    @SpringBootConfiguration
    @AutoConfigurationPackage(basePackages = "stirling.software.proprietary")
    static class TestApp {}
@@ -1,11 +1,13 @@
 package stirling.software.proprietary.storage.service;

 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;

 import java.io.IOException;
@@ -21,6 +23,9 @@ import org.mockito.junit.jupiter.MockitoSettings;
 import org.mockito.quality.Strictness;
 import org.springframework.http.HttpStatus;
 import org.springframework.mock.web.MockMultipartFile;
+import org.springframework.transaction.support.TransactionSynchronization;
+import org.springframework.transaction.support.TransactionSynchronizationManager;
+import org.springframework.transaction.support.TransactionSynchronizationUtils;
 import org.springframework.web.server.ResponseStatusException;

 import stirling.software.common.model.ApplicationProperties;
@@ -105,6 +110,13 @@ class FileStorageServiceTest {
    }

    private FileShare shareFor(StoredFile file, User user, ShareAccessRole role) {
+        FileShare s = legacyShareFor(file, user, role);
+        s.setWriteEnabled(role == ShareAccessRole.EDITOR);
+        return s;
+    }
+
+    /** A share as it exists after upgrading: role set, write never explicitly granted. */
+    private FileShare legacyShareFor(StoredFile file, User user, ShareAccessRole role) {
        FileShare s = new FileShare();
        s.setFile(file);
        s.setSharedWithUser(user);
@@ -655,6 +667,7 @@ class FileStorageServiceTest {
        linkShare.setFile(existing);
        linkShare.setShareToken("token-1");
        linkShare.setAccessRole(ShareAccessRole.EDITOR);
+        linkShare.setWriteEnabled(true);
        when(storageProvider.store(any(), any())).thenReturn(storedObject("new-key"));
        when(storedFileRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
        MockMultipartFile file =
@@ -683,6 +696,236 @@ class FileStorageServiceTest {
                .isEqualTo(403);
    }

+    // replaceFile - write is opt-in per share
+
+    @Test
+    void replaceFile_legacyEditorShare_withoutExplicitWriteGrant_forbidden() {
+        User owner = user(1L);
+        User editor = user(2L);
+        StoredFile existing = ownedFile(owner);
+        FileShare share = legacyShareFor(existing, editor, ShareAccessRole.EDITOR);
+        when(fileShareRepository.findByFileAndSharedWithUser(existing, editor))
+                .thenReturn(Optional.of(share));
+        MockMultipartFile file =
+                new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
+
+        assertThatThrownBy(() -> service.replaceFile(editor, existing, file, null, null))
+                .isInstanceOf(ResponseStatusException.class)
+                .extracting(e -> ((ResponseStatusException) e).getStatusCode().value())
+                .isEqualTo(403);
+        verify(storedFileRepository, never()).save(any());
+    }
+
+    @Test
+    void replaceFileViaShareLink_legacyEditorLink_withoutExplicitWriteGrant_forbidden() {
+        User owner = user(1L);
+        StoredFile existing = ownedFile(owner);
+        FileShare linkShare = new FileShare();
+        linkShare.setFile(existing);
+        linkShare.setShareToken("token-1");
+        linkShare.setAccessRole(ShareAccessRole.EDITOR);
+        MockMultipartFile file =
+                new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
+
+        assertThatThrownBy(() -> service.replaceFileViaShareLink(linkShare, file, null, null, null))
+                .isInstanceOf(ResponseStatusException.class)
+                .extracting(e -> ((ResponseStatusException) e).getStatusCode().value())
+                .isEqualTo(403);
+        verify(storedFileRepository, never()).save(any());
+    }
+
+    @Test
+    void shareWithUser_editorRole_grantsWriteExplicitly() {
+        User owner = user(1L);
+        User target = user(2L);
+        StoredFile file = ownedFile(owner);
+        when(storedFileRepository.findByIdAndOwnerWithShares(100L, owner))
+                .thenReturn(Optional.of(file));
+        when(userRepository.findByUsernameIgnoreCase("user2")).thenReturn(Optional.of(target));
+        when(fileShareRepository.findByFileAndSharedWithUser(file, target))
+                .thenReturn(Optional.empty());
+        when(fileShareRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
+
+        FileShare created = service.shareWithUser(owner, file, "user2", ShareAccessRole.EDITOR);
+
+        assertThat(created.getWriteEnabled()).isTrue();
+    }
+
+    @Test
+    void shareWithUser_viewerRole_doesNotGrantWrite() {
+        User owner = user(1L);
+        User target = user(2L);
+        StoredFile file = ownedFile(owner);
+        when(storedFileRepository.findByIdAndOwnerWithShares(100L, owner))
+                .thenReturn(Optional.of(file));
+        when(userRepository.findByUsernameIgnoreCase("user2")).thenReturn(Optional.of(target));
+        when(fileShareRepository.findByFileAndSharedWithUser(file, target))
+                .thenReturn(Optional.empty());
+        when(fileShareRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
+
+        FileShare created = service.shareWithUser(owner, file, "user2", ShareAccessRole.VIEWER);
+
+        assertThat(created.getWriteEnabled()).isFalse();
+    }
+
+    // replaceFile - only the owner may replace history / audit artifacts
+
+    @Test
+    void replaceFile_editorShare_cannotReplaceHistoryBundle() {
+        User owner = user(1L);
+        User editor = user(2L);
+        StoredFile existing = ownedFile(owner);
+        existing.setHistoryStorageKey("owner-history");
+        FileShare share = shareFor(existing, editor, ShareAccessRole.EDITOR);
+        when(fileShareRepository.findByFileAndSharedWithUser(existing, editor))
+                .thenReturn(Optional.of(share));
+        MockMultipartFile file =
+                new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
+        MockMultipartFile history =
+                new MockMultipartFile("historyBundle", "h.zip", "application/zip", new byte[] {1});
+
+        assertThatThrownBy(() -> service.replaceFile(editor, existing, file, history, null))
+                .isInstanceOf(ResponseStatusException.class)
+                .extracting(e -> ((ResponseStatusException) e).getStatusCode().value())
+                .isEqualTo(403);
+        // The owner's archive must survive an editor attempting to overwrite it.
+        assertThat(existing.getHistoryStorageKey()).isEqualTo("owner-history");
+        verifyNoInteractions(storageProvider);
+    }
+
+    @Test
+    void replaceFile_editorShare_cannotReplaceAuditLog() {
+        User owner = user(1L);
+        User editor = user(2L);
+        StoredFile existing = ownedFile(owner);
+        existing.setAuditLogStorageKey("owner-audit");
+        FileShare share = shareFor(existing, editor, ShareAccessRole.EDITOR);
+        when(fileShareRepository.findByFileAndSharedWithUser(existing, editor))
+                .thenReturn(Optional.of(share));
+        MockMultipartFile file =
+                new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
+        MockMultipartFile audit =
+                new MockMultipartFile("auditLog", "a.json", "application/json", new byte[] {1});
+
+        assertThatThrownBy(() -> service.replaceFile(editor, existing, file, null, audit))
+                .isInstanceOf(ResponseStatusException.class)
+                .extracting(e -> ((ResponseStatusException) e).getStatusCode().value())
+                .isEqualTo(403);
+        assertThat(existing.getAuditLogStorageKey()).isEqualTo("owner-audit");
+        verifyNoInteractions(storageProvider);
+    }
+
+    @Test
+    void replaceFileViaShareLink_cannotReplaceHistoryBundle() {
+        User owner = user(1L);
+        StoredFile existing = ownedFile(owner);
+        existing.setHistoryStorageKey("owner-history");
+        FileShare linkShare = new FileShare();
+        linkShare.setFile(existing);
+        linkShare.setShareToken("token-1");
+        linkShare.setAccessRole(ShareAccessRole.EDITOR);
+        linkShare.setWriteEnabled(true);
+        MockMultipartFile file =
+                new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
+        MockMultipartFile history =
+                new MockMultipartFile("historyBundle", "h.zip", "application/zip", new byte[] {1});
+
+        assertThatThrownBy(
+                        () -> service.replaceFileViaShareLink(linkShare, file, history, null, null))
+                .isInstanceOf(ResponseStatusException.class)
+                .extracting(e -> ((ResponseStatusException) e).getStatusCode().value())
+                .isEqualTo(403);
+        assertThat(existing.getHistoryStorageKey()).isEqualTo("owner-history");
+    }
+
+    @Test
+    void replaceFile_owner_mayStillReplaceHistoryBundle() throws IOException {
+        when(storageProperties.getQuotas()).thenReturn(null);
+        User owner = user(1L);
+        StoredFile existing = ownedFile(owner);
+        existing.setStorageKey("old-key");
+        existing.setHistoryStorageKey("old-history");
+        when(storageProvider.store(any(), any())).thenReturn(storedObject("new-key"));
+        when(storedFileRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
+        MockMultipartFile file =
+                new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
+        MockMultipartFile history =
+                new MockMultipartFile("historyBundle", "h.zip", "application/zip", new byte[] {1});
+
+        assertThatCode(() -> service.replaceFile(owner, existing, file, history, null))
+                .doesNotThrowAnyException();
+    }
+
+    // replaceFile - the superseded blob outlives the transaction
+
+    @Test
+    void replaceFile_previousBlobIsDeletedOnlyAfterCommit() throws IOException {
+        when(storageProperties.getQuotas()).thenReturn(null);
+        User owner = user(1L);
+        StoredFile existing = ownedFile(owner);
+        existing.setStorageKey("old-key");
+        when(storageProvider.store(any(), any())).thenReturn(storedObject("new-key"));
+        when(storedFileRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
+        MockMultipartFile file =
+                new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
+
+        TransactionSynchronizationManager.initSynchronization();
+        try {
+            service.replaceFile(owner, existing, file, null, null);
+            // Still uncommitted: a rollback here would restore the row pointing at old-key.
+            verify(storageProvider, never()).delete("old-key");
+
+            TransactionSynchronizationUtils.triggerAfterCommit();
+            verify(storageProvider).delete("old-key");
+        } finally {
+            TransactionSynchronizationManager.clearSynchronization();
+        }
+    }
+
+    @Test
+    void replaceFile_previousBlobSurvivesWhenTheTransactionRollsBack() throws IOException {
+        when(storageProperties.getQuotas()).thenReturn(null);
+        User owner = user(1L);
+        StoredFile existing = ownedFile(owner);
+        existing.setStorageKey("old-key");
+        when(storageProvider.store(any(), any())).thenReturn(storedObject("new-key"));
+        when(storedFileRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
+        MockMultipartFile file =
+                new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
+
+        TransactionSynchronizationManager.initSynchronization();
+        try {
+            service.replaceFile(owner, existing, file, null, null);
+            TransactionSynchronizationUtils.triggerAfterCompletion(
+                    TransactionSynchronization.STATUS_ROLLED_BACK);
+
+            verify(storageProvider, never()).delete("old-key");
+        } finally {
+            TransactionSynchronizationManager.clearSynchronization();
+        }
+    }
+
+    // replaceFile - a rejected write must not destroy the previous blob
+
+    @Test
+    void replaceFile_versionMismatch_leavesPreviousBlobIntact() {
+        when(storageProperties.getQuotas()).thenReturn(null);
+        User owner = user(1L);
+        StoredFile existing = ownedFile(owner);
+        existing.setContentVersion(5L);
+        existing.setStorageKey("old-key");
+        when(storedFileRepository.bumpContentVersionIfMatches(100L, 4L)).thenReturn(0);
+        MockMultipartFile file =
+                new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
+
+        assertThatThrownBy(() -> service.replaceFile(owner, existing, file, null, null, 4L))
+                .isInstanceOf(ResponseStatusException.class)
+                .extracting(e -> ((ResponseStatusException) e).getStatusCode().value())
+                .isEqualTo(409);
+        assertThat(existing.getStorageKey()).isEqualTo("old-key");
+        verifyNoInteractions(storageProvider);
+    }
+
    @Test
    void replaceFile_editorShare_quotaChargedToOwner() throws IOException {
        when(storageProperties.getQuotas()).thenReturn(quotasProperties);
@@ -16,6 +16,7 @@ interface StoredFileResponse {
  owner?: string | null;
  ownedByCurrentUser?: boolean;
  accessRole?: string | null;
+  canEdit?: boolean;
  shareLinks?: Array<{ token?: string | null }>;
  sharedWithUsers?: string[];
  filePurpose?: string | null;
@@ -185,6 +186,10 @@ export const useFileManager = () => {
                  ? serverFile.ownedByCurrentUser
                  : stub.remoteOwnedByCurrentUser,
              remoteAccessRole: serverFile.accessRole ?? stub.remoteAccessRole,
+              remoteCanEdit:
+                typeof serverFile.canEdit === "boolean"
+                  ? serverFile.canEdit
+                  : stub.remoteCanEdit,
              remoteSharedViaLink: stub.remoteSharedViaLink,
              remoteHasShareLinks: Boolean(serverFile.shareLinks?.length),
              remoteStorageUpdatedAt:
@@ -235,6 +240,8 @@ export const useFileManager = () => {
                  ? file.ownedByCurrentUser
                  : undefined,
              remoteAccessRole: file.accessRole ?? undefined,
+              remoteCanEdit:
+                typeof file.canEdit === "boolean" ? file.canEdit : undefined,
              remoteSharedViaLink: false,
              remoteHasShareLinks: Boolean(file.shareLinks?.length),
            });
@@ -27,11 +27,14 @@ export function canEditSharedFile(file: StirlingFileStub): boolean {
    Boolean(file.remoteSharedViaLink);
  const hasServerRef = Boolean(file.remoteStorageId || file.remoteShareToken);
  const role = (file.remoteAccessRole ?? "viewer").toLowerCase();
+  // Server decides; role is the fallback for stubs cached before canEdit existed.
+  const writable = file.remoteCanEdit ?? role === "editor";
  return (
    isSharedWithUser &&
    hasServerRef &&
    file.remoteOwnedByCurrentUser !== true &&
-    role === "editor"
+    role === "editor" &&
+    writable
  );
 }

@@ -62,6 +65,7 @@ export function useSharedFileActions() {
            remoteOwnerUsername: file.remoteOwnerUsername,
            remoteOwnedByCurrentUser: file.remoteOwnedByCurrentUser,
            remoteAccessRole: file.remoteAccessRole,
+            remoteCanEdit: file.remoteCanEdit,
            remoteSharedViaLink: file.remoteSharedViaLink,
            remoteShareToken: file.remoteShareToken,
            remoteVersionBase: latestVersion ?? undefined,
@@ -162,6 +162,7 @@ class FileStorageService {
      remoteOwnerUsername: stub.remoteOwnerUsername,
      remoteOwnedByCurrentUser: stub.remoteOwnedByCurrentUser,
      remoteAccessRole: stub.remoteAccessRole,
+      remoteCanEdit: stub.remoteCanEdit,
      remoteSharedViaLink: stub.remoteSharedViaLink,
      remoteHasShareLinks: stub.remoteHasShareLinks,
      remoteShareToken: stub.remoteShareToken,
@@ -312,6 +313,7 @@ class FileStorageService {
          remoteOwnerUsername: record.remoteOwnerUsername,
          remoteOwnedByCurrentUser: record.remoteOwnedByCurrentUser,
          remoteAccessRole: record.remoteAccessRole,
+          remoteCanEdit: record.remoteCanEdit,
          remoteSharedViaLink: record.remoteSharedViaLink,
          remoteHasShareLinks: record.remoteHasShareLinks,
          remoteShareToken: record.remoteShareToken,
@@ -374,6 +376,7 @@ class FileStorageService {
              remoteOwnerUsername: record.remoteOwnerUsername,
              remoteOwnedByCurrentUser: record.remoteOwnedByCurrentUser,
              remoteAccessRole: record.remoteAccessRole,
+              remoteCanEdit: record.remoteCanEdit,
              remoteSharedViaLink: record.remoteSharedViaLink,
              remoteHasShareLinks: record.remoteHasShareLinks,
              remoteShareToken: record.remoteShareToken,
@@ -468,6 +471,7 @@ class FileStorageService {
              remoteOwnerUsername: record.remoteOwnerUsername,
              remoteOwnedByCurrentUser: record.remoteOwnedByCurrentUser,
              remoteAccessRole: record.remoteAccessRole,
+              remoteCanEdit: record.remoteCanEdit,
              remoteSharedViaLink: record.remoteSharedViaLink,
              remoteHasShareLinks: record.remoteHasShareLinks,
              remoteShareToken: record.remoteShareToken,
@@ -43,6 +43,7 @@ interface StoredFileResponse {
  owner?: string | null;
  ownedByCurrentUser?: boolean;
  accessRole?: string | null;
+  canEdit?: boolean;
  version?: number | null;
  shareLinks?: Array<{ token?: string | null }>;
  sharedUsers?: Array<{ username?: string | null }>;
@@ -58,6 +59,7 @@ interface AccessedShareLinkResponse {
  owner?: string | null;
  ownedByCurrentUser?: boolean;
  accessRole?: string | null;
+  canEdit?: boolean;
  version?: number | null;
  createdAt?: string | null;
  lastAccessedAt?: string | null;
@@ -193,6 +195,10 @@ export async function reconcileServerFiles(
            ? serverFile.ownedByCurrentUser
            : stub.remoteOwnedByCurrentUser,
        remoteAccessRole: serverFile.accessRole ?? stub.remoteAccessRole,
+        remoteCanEdit:
+          typeof serverFile.canEdit === "boolean"
+            ? serverFile.canEdit
+            : stub.remoteCanEdit,
        remoteSharedViaLink: stub.remoteSharedViaLink,
        remoteHasShareLinks: Boolean(serverFile.shareLinks?.length),
        remoteHasUserShares: Boolean(
@@ -250,6 +256,8 @@ export async function reconcileServerFiles(
            ? file.ownedByCurrentUser
            : undefined,
        remoteAccessRole: file.accessRole ?? undefined,
+        remoteCanEdit:
+          typeof file.canEdit === "boolean" ? file.canEdit : undefined,
        remoteVersionLatest:
          typeof file.version === "number" ? file.version : undefined,
        remoteSharedViaLink: false,
@@ -371,6 +379,8 @@ export async function reconcileServerFiles(
        remoteOwnerUsername: link.owner ?? undefined,
        remoteOwnedByCurrentUser: false,
        remoteAccessRole: link.accessRole ?? undefined,
+        remoteCanEdit:
+          typeof link.canEdit === "boolean" ? link.canEdit : undefined,
        remoteVersionLatest:
          typeof link.version === "number" ? link.version : undefined,
        remoteSharedViaLink: true,
@@ -80,6 +80,7 @@ export interface BaseFileMetadata {
  remoteOwnerUsername?: string; // Server-side owner username (if known)
  remoteOwnedByCurrentUser?: boolean; // Ownership flag for server files
  remoteAccessRole?: string; // Access role for shared server files
+  remoteCanEdit?: boolean; // Server's write decision; editor role alone is not enough
  remoteSharedViaLink?: boolean; // True when imported from a share link
  remoteHasShareLinks?: boolean; // True when owner has shared this file
  remoteHasUserShares?: boolean; // True when owner has invited specific users
@@ -30,6 +30,7 @@ interface ShareLinkMetadata {
  owner?: string | null;
  ownedByCurrentUser?: boolean;
  accessRole?: string | null;
+  canEdit?: boolean;
  expiresAt?: string;
 }

@@ -133,6 +134,10 @@ export default function ShareLinkLoader({ token }: ShareLinkLoaderProps) {
                remoteOwnerUsername: shareMetadata?.owner ?? undefined,
                remoteOwnedByCurrentUser: false,
                remoteAccessRole: shareMetadata?.accessRole ?? undefined,
+                remoteCanEdit:
+                  typeof shareMetadata?.canEdit === "boolean"
+                    ? shareMetadata.canEdit
+                    : undefined,
                remoteSharedViaLink: true,
                remoteHasShareLinks: false,
                remoteShareToken: shareMetadata?.shareToken || normalizedToken,
@@ -200,6 +205,10 @@ export default function ShareLinkLoader({ token }: ShareLinkLoaderProps) {
            remoteOwnerUsername: shareMetadata?.owner ?? undefined,
            remoteOwnedByCurrentUser: false,
            remoteAccessRole: shareMetadata?.accessRole ?? undefined,
+            remoteCanEdit:
+              typeof shareMetadata?.canEdit === "boolean"
+                ? shareMetadata.canEdit
+                : undefined,
            remoteSharedViaLink: true,
            remoteHasShareLinks: false,
            remoteShareToken: shareMetadata?.shareToken || normalizedToken,
@@ -18,6 +18,7 @@ export interface ShareLinkMetadata {
  owner?: string | null;
  ownedByCurrentUser?: boolean;
  accessRole?: string | null;
+  canEdit?: boolean;
  version?: number | null;
  createdAt?: string;
  expiresAt?: string;
@@ -95,6 +96,10 @@ export async function importShareLinkToWorkbench(
        remoteOwnerUsername: shareMetadata?.owner ?? undefined,
        remoteOwnedByCurrentUser: false,
        remoteAccessRole: shareMetadata?.accessRole ?? undefined,
+        remoteCanEdit:
+          typeof shareMetadata?.canEdit === "boolean"
+            ? shareMetadata.canEdit
+            : undefined,
        remoteSharedViaLink: true,
        remoteHasShareLinks: false,
        remoteShareToken: shareMetadata?.shareToken || token,
@@ -166,6 +171,10 @@ export async function importShareLinkToWorkbench(
      remoteOwnerUsername: shareMetadata?.owner ?? undefined,
      remoteOwnedByCurrentUser: false,
      remoteAccessRole: shareMetadata?.accessRole ?? undefined,
+      remoteCanEdit:
+        typeof shareMetadata?.canEdit === "boolean"
+          ? shareMetadata.canEdit
+          : undefined,
      remoteSharedViaLink: true,
      remoteHasShareLinks: false,
      remoteShareToken: shareMetadata?.shareToken || token,