Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-835 Add permission-based access to warps #856

Merged
merged 24 commits into from
Jan 12, 2025
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
fca57f8
Add possibility to manage warp permissions
eripe14 Oct 1, 2024
65e7564
Change warp permission feature
eripe14 Oct 8, 2024
374fc89
Add config description
eripe14 Oct 8, 2024
f1a6ada
Resolve CitralFlo suggestion
eripe14 Oct 8, 2024
7eb3bfc
Merge branch 'master' into add-possibility-to-set-warp-permissions
vLuckyyy Jan 10, 2025
1dbc78f
Merge branch 'master' into add-possibility-to-set-warp-permissions
vLuckyyy Jan 12, 2025
2e80db7
Refactor and enhance warp permission handling
vLuckyyy Jan 12, 2025
0aaf226
Update eternalcore-core/src/main/java/com/eternalcode/core/translatio…
vLuckyyy Jan 12, 2025
bf890c7
Update eternalcore-core/src/main/java/com/eternalcode/core/translatio…
vLuckyyy Jan 12, 2025
d11676f
Update eternalcore-core/src/main/java/com/eternalcode/core/translatio…
vLuckyyy Jan 12, 2025
2804b53
Update eternalcore-core/src/main/java/com/eternalcode/core/feature/wa…
vLuckyyy Jan 12, 2025
a8ac2e2
Update eternalcore-core/src/main/java/com/eternalcode/core/feature/wa…
vLuckyyy Jan 12, 2025
708224f
Update eternalcore-core/src/main/java/com/eternalcode/core/feature/wa…
vLuckyyy Jan 12, 2025
2a67403
Fix repository name, check if permission already exist.
vLuckyyy Jan 12, 2025
95f8ed3
Follow mr. @Rollczi review.
vLuckyyy Jan 12, 2025
60b4c11
Follow mr. @Rollczi review. v2
vLuckyyy Jan 12, 2025
f86cc47
Fix synchronized
Rollczi Jan 12, 2025
a84633d
Merge remote-tracking branch 'origin/add-possibility-to-set-warp-perm…
Rollczi Jan 12, 2025
e68afcc
Fix merge
Rollczi Jan 12, 2025
a67693c
Fix events life cycle
Rollczi Jan 12, 2025
c9d7daf
Fix gui permissions
Rollczi Jan 12, 2025
c6115be
Fix cdn
Rollczi Jan 12, 2025
cec9d90
Fix cdn
Rollczi Jan 12, 2025
72feb45
Fix cdn.
vLuckyyy Jan 12, 2025
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 @@ -2,10 +2,16 @@

import org.bukkit.Location;

import java.util.List;

public interface Warp {

Location getLocation();

String getName();

List<String> getPermissions();
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider returning an unmodifiable list of permissions

To prevent accidental modifications, wrap the permissions list with Collections.unmodifiableList().


boolean hasPermission(String permission);

Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add Javadoc to describe the new methods

Please add documentation to explain:

  • What kind of permissions are expected
  • Whether the permission check is case-sensitive
  • Example permission format (e.g., eternalcore.warp.vipzone)
+/**
+ * @return An unmodifiable list of permissions required for this warp
+ */
 List<String> getPermissions();

+/**
+ * @param permission The permission to check (e.g., eternalcore.warp.vipzone)
+ * @return true if this warp has the specified permission
+ */
 boolean hasPermission(String permission);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
List<String> getPermissions();
boolean hasPermission(String permission);
/**
* @return An unmodifiable list of permissions required for this warp
*/
List<String> getPermissions();
/**
* @param permission The permission to check (e.g., eternalcore.warp.vipzone)
* @return true if this warp has the specified permission
*/
boolean hasPermission(String permission);

}
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,22 @@

import java.util.Collection;
import java.util.Optional;
import org.jetbrains.annotations.ApiStatus.Experimental;

public interface WarpService {

Warp createWarp(String name, Location location);

void removeWarp(String warp);

boolean warpExists(String name);
void addPermissions(String warp, String... permissions);

Optional<Warp> findWarp(String name);
@Experimental
void removePermission(String warp, String permission);
vLuckyyy marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider allowing removePermission to accept multiple permissions.

This would make it consistent with addPermissions.


vLuckyyy marked this conversation as resolved.
Show resolved Hide resolved
boolean isExist(String name);
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider renaming isExist to exists for clarity.

This name is more conventional and readable.


Collection<String> getNamesOfWarps();
Optional<Warp> findWarp(String name);

boolean hasWarps();
Collection<Warp> getWarps();
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package com.eternalcode.core.configuration.implementation;

import com.eternalcode.commons.bukkit.position.Position;
import com.eternalcode.core.configuration.ReloadableConfig;
import com.eternalcode.core.injector.annotations.component.ConfigurationFile;
import com.eternalcode.commons.bukkit.position.Position;
import net.dzikoysk.cdn.entity.Description;
import net.dzikoysk.cdn.entity.Exclude;
import net.dzikoysk.cdn.source.Resource;
Expand All @@ -21,7 +21,8 @@ public class LocationsConfiguration implements ReloadableConfig {
@Description("# This is spawn location, for your own safety, please don't touch it.")
public Position spawn = EMPTY_POSITION;

@Description("# These are warp locations, for your own safety, please don't touch it.")
@Description("# Warps now are stored in warps.yml. This is deprecated.")
@Deprecated
vLuckyyy marked this conversation as resolved.
Show resolved Hide resolved
public Map<String, Position> warps = new HashMap<>();
vLuckyyy marked this conversation as resolved.
Show resolved Hide resolved

@Description("# This is jail location, for your own safety, please don't touch it.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,13 @@
import com.eternalcode.core.feature.afk.AfkSettings;
import com.eternalcode.core.feature.automessage.AutoMessageSettings;
import com.eternalcode.core.feature.chat.ChatSettings;
import com.eternalcode.core.feature.jail.JailSettings;
import com.eternalcode.core.feature.helpop.HelpOpSettings;
import com.eternalcode.core.feature.jail.JailSettings;
import com.eternalcode.core.feature.randomteleport.RandomTeleportSettingsImpl;
import com.eternalcode.core.feature.spawn.SpawnSettings;
import com.eternalcode.core.injector.annotations.Bean;
import com.eternalcode.core.injector.annotations.component.ConfigurationFile;
import com.eternalcode.core.feature.teleportrequest.TeleportRequestSettings;
import java.util.LinkedHashMap;
import java.util.Set;
import net.dzikoysk.cdn.entity.Contextual;
import net.dzikoysk.cdn.entity.Description;
import net.dzikoysk.cdn.entity.Exclude;
Expand All @@ -24,7 +22,9 @@

import java.io.File;
import java.time.Duration;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Set;

@ConfigurationFile
public class PluginConfiguration implements ReloadableConfig {
Expand Down Expand Up @@ -352,7 +352,6 @@ public static class Warp {

@Description("# Texture of the item (only for PLAYER_HEAD material)")
public String itemTexture = "eyJ0ZXh0dXJlcyI6eyJTS0lOIjp7InVybCI6Imh0dHA6Ly90ZXh0dXJlcy5taW5lY3JhZnQubmV0L3RleHR1cmUvNzk4ODVlODIzZmYxNTkyNjdjYmU4MDkwOTNlMzNhNDc2ZTI3NDliNjU5OGNhNGEyYTgxZWU2OTczODAzZmI2NiJ9fX0=";

}

@Description({ " ", "# Butcher" })
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,23 @@

import com.eternalcode.commons.bukkit.position.Position;
import com.eternalcode.commons.bukkit.position.PositionAdapter;
import java.util.ArrayList;
import org.bukkit.Location;

class WarpImpl implements Warp {
import java.util.Collections;
import java.util.List;

@SuppressWarnings("LombokSetterMayBeUsed")
public class WarpImpl implements Warp {

private final String name;
private final Position position;
private List<String> permissions;

WarpImpl(String name, Position position) {
public WarpImpl(String name, Position position, List<String> permissions) {
this.name = name;
this.position = position;
this.permissions = new ArrayList<>(permissions);
Comment on lines +17 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add a null check for permissions in the constructor.

This helps prevent potential NullPointerException.

}

@Override
Expand All @@ -23,4 +30,18 @@ public String getName() {
public Location getLocation() {
return PositionAdapter.convert(this.position);
}

@Override
public List<String> getPermissions() {
return Collections.unmodifiableList(this.permissions);
}

@Override
public boolean hasPermission(String permission) {
return this.permissions.contains(permission);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add null check in hasPermission method.

The method should handle null input gracefully:

 public boolean hasPermission(String permission) {
+    if (permission == null) {
+        return false;
+    }
     return this.permissions.contains(permission);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Override
public boolean hasPermission(String permission) {
return this.permissions.contains(permission);
}
@Override
public boolean hasPermission(String permission) {
if (permission == null) {
return false;
}
return this.permissions.contains(permission);
}


public void setPermissions(List<String> permissions) {
this.permissions = permissions;
}
vLuckyyy marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,16 @@
import dev.triumphteam.gui.builder.item.ItemBuilder;
import dev.triumphteam.gui.guis.Gui;
import dev.triumphteam.gui.guis.GuiItem;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import net.kyori.adventure.text.Component;
import net.kyori.adventure.text.minimessage.MiniMessage;
import org.bukkit.Material;
import org.bukkit.Server;
import org.bukkit.entity.Player;

import java.util.Collections;
import java.util.List;
import java.util.Optional;

@Service
public class WarpInventory {

Expand Down Expand Up @@ -80,7 +81,6 @@ private Gui createInventory(Language language) {
}
}


Gui gui = Gui.gui()
.title(this.miniMessage.deserialize(warpSection.title()))
.rows(rowsCount)
Expand Down Expand Up @@ -198,13 +198,11 @@ public void openInventory(Player player, Language language) {
}

public void addWarp(Warp warp) {

if (!this.warpManager.warpExists(warp.getName())) {
if (!this.warpManager.isExist(warp.getName())) {
return;
}

for (Language language : this.translationManager.getAvailableLanguages()) {

AbstractTranslation translation = (AbstractTranslation) this.translationManager.getMessages(language);
Translation.WarpSection.WarpInventorySection warpSection = translation.warp().warpInventory();

Expand Down Expand Up @@ -245,7 +243,7 @@ public void addWarp(Warp warp) {

public boolean removeWarp(String warpName) {

if (!this.warpManager.warpExists(warpName)) {
if (!this.warpManager.isExist(warpName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Consider renaming for clarity.

The logic is correct, but consider renaming isExist to exists for better readability.

-        if (!this.warpManager.isExist(warpName)) {
+        if (!this.warpManager.exists(warpName)) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!this.warpManager.isExist(warpName)) {
if (!this.warpManager.exists(warpName)) {

return false;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package com.eternalcode.core.feature.warp;

import com.eternalcode.annotations.scan.feature.FeatureDocs;
import com.eternalcode.core.configuration.implementation.PluginConfiguration;
import com.eternalcode.core.feature.warp.event.PreWarpTeleportEvent;
import com.eternalcode.core.injector.annotations.Inject;
import com.eternalcode.core.injector.annotations.component.Controller;
import com.eternalcode.core.notice.NoticeService;
import org.bukkit.entity.Player;
import org.bukkit.event.EventHandler;
import org.bukkit.event.Listener;

import java.util.List;
import java.util.Optional;
import java.util.UUID;

@FeatureDocs(
description = "Check if a player has permission to use a specific warp",
name = "Warp permission"
)
@Controller
public class WarpPermissionController implements Listener {

private final NoticeService noticeService;
private final PluginConfiguration config;

@Inject
public WarpPermissionController(NoticeService noticeService, PluginConfiguration config) {
this.noticeService = noticeService;
this.config = config;
}

@EventHandler
void onWarpPreTeleportation(PreWarpTeleportEvent event) {
Player player = event.getPlayer();
UUID uniqueId = player.getUniqueId();
Warp warp = event.getWarp();

this.checkWarpPermission(event, warp, player, uniqueId);
}

private void checkWarpPermission(PreWarpTeleportEvent event, Warp warp, Player player, UUID uniqueId) {
vLuckyyy marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

metoda ma side efekty, i w sumie nie ma powodu żeby to rozdzielaż na 2 metody chyba że dla isPlayerAllowedToUseWarp etc

List<String> permissions = warp.getPermissions();
vLuckyyy marked this conversation as resolved.
Show resolved Hide resolved
Optional<String> isPlayerAllowedToUseWarp = permissions
.stream()
.filter(player::hasPermission)
.findAny();

if (isPlayerAllowedToUseWarp.isPresent() || permissions.isEmpty()) {
return;
}

event.setCancelled(true);

this.noticeService.create()
Copy link
Member

Choose a reason for hiding this comment

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

i trochę to głupie bo anulujemy event wysyłamy wiadomość a ktoś może go odanulować a wiadomość zostanie może da się to ogarnąć?

.player(uniqueId)
.placeholder("{WARP}", warp.getName())
eripe14 marked this conversation as resolved.
Show resolved Hide resolved
.placeholder("{PERMISSIONS}", String.join(this.config.format.separator, permissions))
.notice(translation -> translation.warp().noPermission())
.send();
}

}

This file was deleted.

Loading
Loading