-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
Changes from 15 commits
fca57f8
65e7564
374fc89
f1a6ada
7eb3bfc
1dbc78f
2e80db7
0aaf226
bf890c7
d11676f
2804b53
a8ac2e2
708224f
2a67403
95f8ed3
60b4c11
f86cc47
a84633d
e68afcc
a67693c
c9d7daf
c6115be
cec9d90
72feb45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,10 +2,16 @@ | |||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import org.bukkit.Location; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import java.util.List; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public interface Warp { | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
Location getLocation(); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
String getName(); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
List<String> getPermissions(); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
boolean hasPermission(String permission); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
+/**
+ * @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
Suggested change
|
||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consider allowing This would make it consistent with |
||
|
||
vLuckyyy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
boolean isExist(String name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consider renaming This name is more conventional and readable. |
||
|
||
Collection<String> getNamesOfWarps(); | ||
Optional<Warp> findWarp(String name); | ||
|
||
boolean hasWarps(); | ||
Collection<Warp> getWarps(); | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Add a null check for This helps prevent potential |
||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
@Override | ||||||||||||||||||||||||
|
@@ -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); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
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 | ||||
---|---|---|---|---|---|---|
|
@@ -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 { | ||||||
|
||||||
|
@@ -80,7 +81,6 @@ private Gui createInventory(Language language) { | |||||
} | ||||||
} | ||||||
|
||||||
|
||||||
Gui gui = Gui.gui() | ||||||
.title(this.miniMessage.deserialize(warpSection.title())) | ||||||
.rows(rowsCount) | ||||||
|
@@ -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(); | ||||||
|
||||||
|
@@ -245,7 +243,7 @@ public void addWarp(Warp warp) { | |||||
|
||||||
public boolean removeWarp(String warpName) { | ||||||
|
||||||
if (!this.warpManager.warpExists(warpName)) { | ||||||
if (!this.warpManager.isExist(warpName)) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 - if (!this.warpManager.isExist(warpName)) {
+ if (!this.warpManager.exists(warpName)) { 📝 Committable suggestion
Suggested change
|
||||||
return false; | ||||||
} | ||||||
|
||||||
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment.
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().