Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,8 @@ public void onEnable() {
itemStorageManager,
parcelDispatchService,
parcelContentManager,
deliveryManager
deliveryManager,
config.settings.allowCollectingFromAnyLocker
);

MainGui mainGUI = new MainGui(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,14 @@ public static class Settings extends OkaeriConfig {
@Comment({"", "# Maximum number of parcels that can be stored in a single locker"})
public int maxParcelsPerLocker = 30;

@Comment({
"",
"# Whether a parcel can be collected from any locker instead of only its destination locker.",
"# false (default): parcels can only be collected from the locker they were sent to.",
"# true: keeps the legacy behavior where every parcel can be collected from every locker."
})
public boolean allowCollectingFromAnyLocker = false;

@Comment({"", "# Small parcel fee in in-game currency"})
public double smallParcelFee = 10.0;

Expand Down
23 changes: 22 additions & 1 deletion src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.eternalcode.parcellockers.user.User;
import com.eternalcode.parcellockers.user.UserManager;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.UUID;
import java.util.concurrent.CompletableFuture;
Expand All @@ -33,6 +34,7 @@ public class GuiManager {
private final ParcelDispatchService parcelDispatchService;
private final ParcelContentManager parcelContentManager;
private final DeliveryManager deliveryManager;
private final boolean allowCollectingFromAnyLocker;

public GuiManager(
ParcelService parcelService,
Expand All @@ -41,7 +43,8 @@ public GuiManager(
ItemStorageManager itemStorageManager,
ParcelDispatchService parcelDispatchService,
ParcelContentManager parcelContentManager,
DeliveryManager deliveryManager
DeliveryManager deliveryManager,
boolean allowCollectingFromAnyLocker
) {
this.parcelService = parcelService;
this.lockerManager = lockerManager;
Expand All @@ -50,6 +53,24 @@ public GuiManager(
this.parcelDispatchService = parcelDispatchService;
this.parcelContentManager = parcelContentManager;
this.deliveryManager = deliveryManager;
this.allowCollectingFromAnyLocker = allowCollectingFromAnyLocker;
}

/**
* Returns the delivered parcels the receiver may collect at the locker they are interacting with.
* When {@code allowCollectingFromAnyLocker} is enabled the locker restriction is dropped.
*
* <p>{@code getCollectible} treats a {@code null} destination locker as "collect from any locker",
* so when the restriction is active a concrete {@code currentLocker} is required — passing
* {@code null} here would silently drop the restriction and let the receiver collect from any locker.
*/
public CompletableFuture<PageResult<Parcel>> getCollectibleParcels(UUID receiver, UUID currentLocker, Page page) {
if (this.allowCollectingFromAnyLocker) {
return this.parcelService.getCollectible(receiver, null, page);
}
Objects.requireNonNull(currentLocker,
"currentLocker must not be null when collection is restricted to the destination locker");
return this.parcelService.getCollectible(receiver, currentLocker, page);
}
Comment on lines +67 to 74

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

If allowCollectingFromAnyLocker is false but currentLocker is null, destinationLocker will be set to null. In ParcelRepositoryOrmLite#findCollectible, a null destination locker bypasses the locker restriction filter entirely, allowing the player to collect parcels from any locker. To prevent this security/logic bypass, we should ensure that a non-existent/dummy UUID is used when currentLocker is null and restriction is active.

Suggested change
public CompletableFuture<PageResult<Parcel>> getCollectibleParcels(UUID receiver, UUID currentLocker, Page page) {
UUID destinationLocker = this.allowCollectingFromAnyLocker ? null : currentLocker;
return this.parcelService.getCollectible(receiver, destinationLocker, page);
}
public CompletableFuture<PageResult<Parcel>> getCollectibleParcels(UUID receiver, UUID currentLocker, Page page) {
UUID destinationLocker = this.allowCollectingFromAnyLocker ? null : (currentLocker != null ? currentLocker : new UUID(0L, 0L));
return this.parcelService.getCollectible(receiver, destinationLocker, page);
}


public void sendParcel(Player sender, Parcel parcel, List<ItemStack> items) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import com.eternalcode.parcellockers.gui.GuiView;
import com.eternalcode.parcellockers.gui.PaginatedGuiRefresher;
import com.eternalcode.parcellockers.parcel.Parcel;
import com.eternalcode.parcellockers.parcel.ParcelStatus;
import com.eternalcode.parcellockers.parcel.util.PlaceholderUtil;
import com.eternalcode.parcellockers.shared.Page;
import com.eternalcode.parcellockers.util.MaterialUtil;
Expand All @@ -18,6 +17,7 @@
import dev.triumphteam.gui.guis.PaginatedGui;
import java.util.ArrayList;
import java.util.List;
import java.util.UUID;
import java.util.concurrent.CompletableFuture;
import java.util.function.Supplier;
import net.kyori.adventure.text.Component;
Expand All @@ -35,17 +35,20 @@ public class CollectionGui implements GuiView {
private final Scheduler scheduler;
private final GuiManager guiManager;
private final MiniMessage miniMessage;
private final UUID currentLocker;

public CollectionGui(
GuiSettings guiSettings,
Scheduler scheduler,
GuiManager guiManager,
MiniMessage miniMessage
MiniMessage miniMessage,
UUID currentLocker
) {
this.guiSettings = guiSettings;
this.scheduler = scheduler;
this.guiManager = guiManager;
this.miniMessage = miniMessage;
this.currentLocker = currentLocker;
}

@Override
Expand All @@ -66,7 +69,7 @@ public void show(Player player, Page page) {

this.setupStaticItems(player, gui);

this.guiManager.getParcelsByReceiver(player.getUniqueId(), page).thenAccept(result -> {
this.guiManager.getCollectibleParcels(player.getUniqueId(), this.currentLocker, page).thenAccept(result -> {
if (result == null || result.items().isEmpty()) {
gui.setItem(22, this.guiSettings.noParcelsItem.toGuiItem());
this.scheduler.run(() -> gui.open(player));
Expand All @@ -78,7 +81,6 @@ public void show(Player player, Page page) {
this.setupNavigation(gui, page, result, player, this.guiSettings);

result.items().stream()
.filter(parcel -> parcel.status() == ParcelStatus.DELIVERED)
.map(parcel -> this.createParcelItemAsync(parcel, parcelItem, player, refresher))
.collect(CompletableFutures.joinList())
.thenAccept(suppliers -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ public void show(Player player, UUID entryLocker) {
this.guiSettings,
this.scheduler,
this.guiManager,
this.miniMessage
this.miniMessage,
entryLocker
);

gui.setItem(21, this.guiSettings.parcelLockerCollectItem.toGuiItem(event -> collectionGui.show(player)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,18 @@ public interface ParcelRepository {

CompletableFuture<PageResult<Parcel>> findByReceiver(UUID receiver, Page page);

/**
* Finds the parcels a receiver is allowed to collect: those addressed to them whose status is
* {@link com.eternalcode.parcellockers.parcel.ParcelStatus#DELIVERED}. Filtering happens in the
* query so pagination operates on the eligible set rather than on a raw receiver page.
*
* @param receiver the receiver whose parcels are collected
* @param destinationLocker when non-null, only parcels addressed to this locker are returned;
* when null, delivered parcels from any locker are returned
* @param page the requested page
*/
CompletableFuture<PageResult<Parcel>> findCollectible(UUID receiver, UUID destinationLocker, Page page);

/**
* Counts the parcels currently occupying a destination locker. Collected parcels are removed
* from storage, so every parcel addressed to the locker (in-transit or delivered) occupies a
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.eternalcode.parcellockers.database.DatabaseManager;
import com.eternalcode.parcellockers.database.wrapper.AbstractRepositoryOrmLite;
import com.eternalcode.parcellockers.parcel.Parcel;
import com.eternalcode.parcellockers.parcel.ParcelStatus;
import com.eternalcode.parcellockers.shared.Page;
import com.eternalcode.parcellockers.shared.PageResult;
import java.util.List;
Expand All @@ -17,6 +18,7 @@ public class ParcelRepositoryOrmLite extends AbstractRepositoryOrmLite implement
private static final String RECEIVER_COLUMN = "receiver";
private static final String SENDER_COLUMN = "sender";
private static final String DESTINATION_LOCKER_COLUMN = "destination_locker";
private static final String STATUS_COLUMN = "status";

public ParcelRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) {
super(databaseManager, scheduler);
Expand Down Expand Up @@ -78,6 +80,22 @@ public CompletableFuture<PageResult<Parcel>> findByReceiver(UUID receiver, Page
return this.findByPaged(receiver, page, RECEIVER_COLUMN);
}

@Override
public CompletableFuture<PageResult<Parcel>> findCollectible(UUID receiver, UUID destinationLocker, Page page) {
Objects.requireNonNull(receiver, "Receiver UUID cannot be null");
Objects.requireNonNull(page, "Page cannot be null");
return this.queryPage(ParcelTable.class, page, builder -> {
var where = builder.where()
.eq(RECEIVER_COLUMN, receiver)
.and()
.eq(STATUS_COLUMN, ParcelStatus.DELIVERED);
if (destinationLocker != null) {
where.and().eq(DESTINATION_LOCKER_COLUMN, destinationLocker);
}
return builder;
}, ParcelTable::toParcel);
}

@Override
public CompletableFuture<Integer> countParcelsByDestinationLocker(UUID destinationLocker) {
Objects.requireNonNull(destinationLocker, "Destination locker UUID cannot be null");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ public interface ParcelService {

CompletableFuture<PageResult<Parcel>> getByReceiver(UUID receiver, Page page);

/**
* Returns the delivered parcels a receiver may collect, optionally restricted to a single
* destination locker. Filtering is applied in the query so pagination stays consistent.
*
* @param destinationLocker the locker to collect from, or null to allow any locker
*/
CompletableFuture<PageResult<Parcel>> getCollectible(UUID receiver, UUID destinationLocker, Page page);

CompletableFuture<PageResult<Parcel>> getAll(Page page);

CompletableFuture<Boolean> delete(UUID uuid);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,18 @@ public CompletableFuture<PageResult<Parcel>> getByReceiver(UUID receiver, Page p
});
}

@Override
public CompletableFuture<PageResult<Parcel>> getCollectible(UUID receiver, UUID destinationLocker, Page page) {
Objects.requireNonNull(receiver, "Receiver UUID cannot be null");
Objects.requireNonNull(page, "Page cannot be null");

return this.parcelRepository.findCollectible(receiver, destinationLocker, page)
.thenApply(result -> {
result.items().forEach(parcel -> this.parcelsByUuid.put(parcel.uuid(), parcel));
return result;
});
}

@Override
public CompletableFuture<PageResult<Parcel>> getAll(Page page) {
Objects.requireNonNull(page, "Page cannot be null");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package com.eternalcode.parcellockers.database;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import com.eternalcode.parcellockers.TestScheduler;
import com.eternalcode.parcellockers.configuration.implementation.PluginConfig;
import com.eternalcode.parcellockers.parcel.Parcel;
import com.eternalcode.parcellockers.parcel.ParcelSize;
import com.eternalcode.parcellockers.parcel.ParcelStatus;
import com.eternalcode.parcellockers.parcel.repository.ParcelRepository;
import com.eternalcode.parcellockers.parcel.repository.ParcelRepositoryOrmLite;
import com.eternalcode.parcellockers.shared.Page;
import com.eternalcode.parcellockers.shared.PageResult;
import java.nio.file.Path;
import java.sql.SQLException;
import java.util.UUID;
import java.util.logging.Logger;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import org.testcontainers.containers.MySQLContainer;
import org.testcontainers.junit.jupiter.Container;
import org.testcontainers.junit.jupiter.Testcontainers;
import org.testcontainers.utility.DockerImageName;

@Testcontainers(disabledWithoutDocker = true)
class ParcelFindCollectibleIntegrationTest extends IntegrationTestSpec {

@Container
private static final MySQLContainer<?> mySQLContainer = new MySQLContainer<>(DockerImageName.parse("mysql:latest"));

@TempDir
private Path tempDir;

private DatabaseManager databaseManager;

private ParcelRepository repository() throws SQLException {
PluginConfig config = new PluginConfig();
config.settings.databaseType = DatabaseType.MYSQL;
config.settings.host = mySQLContainer.getHost();
config.settings.port = String.valueOf(mySQLContainer.getFirstMappedPort());
config.settings.databaseName = mySQLContainer.getDatabaseName();
config.settings.user = mySQLContainer.getUsername();
config.settings.password = mySQLContainer.getPassword();

DatabaseManager databaseManager = new DatabaseManager(config, Logger.getLogger("ParcelLockers"), this.tempDir.toFile());
databaseManager.connect();
this.databaseManager = databaseManager;

return new ParcelRepositoryOrmLite(databaseManager, new TestScheduler());
}

private void save(ParcelRepository repository, UUID receiver, UUID destinationLocker, ParcelStatus status) {
this.await(repository.save(new Parcel(
UUID.randomUUID(), UUID.randomUUID(), "p", "d", false,
receiver, ParcelSize.SMALL, UUID.randomUUID(), destinationLocker, status)));
}

@Test
void findCollectibleReturnsOnlyDeliveredParcelsForTheGivenLockerAndReceiver() throws SQLException {
ParcelRepository repository = this.repository();

UUID receiver = UUID.randomUUID();
UUID locker = UUID.randomUUID();
UUID otherLocker = UUID.randomUUID();

for (int i = 0; i < 3; i++) {
this.save(repository, receiver, locker, ParcelStatus.DELIVERED);
}
this.save(repository, receiver, locker, ParcelStatus.SENT); // not yet delivered
this.save(repository, receiver, otherLocker, ParcelStatus.DELIVERED); // different locker
this.save(repository, UUID.randomUUID(), locker, ParcelStatus.DELIVERED); // different receiver

// Pagination must count only the 3 eligible parcels, not the raw receiver page.
PageResult<Parcel> firstPage = this.await(repository.findCollectible(receiver, locker, new Page(0, 2)));
assertEquals(2, firstPage.items().size());
assertTrue(firstPage.hasNextPage());

PageResult<Parcel> secondPage = this.await(repository.findCollectible(receiver, locker, new Page(1, 2)));
assertEquals(1, secondPage.items().size());
assertFalse(secondPage.hasNextPage());
}

@Test
void findCollectibleWithNullLockerReturnsDeliveredParcelsFromAnyLocker() throws SQLException {
ParcelRepository repository = this.repository();

UUID receiver = UUID.randomUUID();

this.save(repository, receiver, UUID.randomUUID(), ParcelStatus.DELIVERED);
this.save(repository, receiver, UUID.randomUUID(), ParcelStatus.DELIVERED);
this.save(repository, receiver, UUID.randomUUID(), ParcelStatus.SENT); // excluded by status
this.save(repository, UUID.randomUUID(), UUID.randomUUID(), ParcelStatus.DELIVERED); // other receiver

PageResult<Parcel> page = this.await(repository.findCollectible(receiver, null, new Page(0, 10)));
assertEquals(2, page.items().size());
assertFalse(page.hasNextPage());
assertTrue(page.items().stream().allMatch(parcel -> parcel.receiver().equals(receiver)));
assertTrue(page.items().stream().allMatch(parcel -> parcel.status() == ParcelStatus.DELIVERED));
}

@AfterEach
void tearDown() {
if (this.databaseManager != null) {
this.databaseManager.disconnect();
}
}
}
Loading