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

Improved PermissionResolver and added support for LuckPerms #519

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

huanmeng-qwq
Copy link
Contributor

This PR primarily adds permission check support to Fabric. Since Fabric API lacks an API for permission validation, I moved the PermissionResolver to the core module, as it is used by both platforms (Minestom and Fabric). I also added a new litecommands-luckperms module that depends on luckperms-api to implement the PermissionResolver interface, enabling LuckPerms-based permission checks through this module.

The reason for this PR is that I discovered Fabric can only implement hasPermission via Mixin.

@Rollczi
Copy link
Owner

Rollczi commented Feb 16, 2025

Hi, I'm sorry for the delay, I haven't had much time lately, I had to catch up on a lot of things, I'm still wondering in what form we can implement it, but the assumptions are good

@huanmeng-qwq
Copy link
Contributor Author

I guess we should be able to refer to adding dependencies on litecommands-luckperms and luckperms-api like litecommands-adventure-platform does, and then set the PermissionResolver in the platform's settings

@huanmeng-qwq
Copy link
Contributor Author

I see you have two PermissionResolver classes:

  1. dev.rollczi.litecommands.settings.PermissionResolver
  2. dev.rollczi.litecommands.permission.PermissionResolver
    Should we use the second one?

@Rollczi
Copy link
Owner

Rollczi commented Mar 2, 2025

I see you have two PermissionResolver classes:

  1. dev.rollczi.litecommands.settings.PermissionResolver
  2. dev.rollczi.litecommands.permission.PermissionResolver
    Should we use the second one?

this is still working in progress, I've been making changes locally for a few weeks now to finally remove hasPermission. so we don't have to do any tricks e.g. minestom

it is possible that such major changes will be the beginning of version 4.x
I didn't plan it, but the system still had to be rewritten at some point

Your changes are a flashpoint for me

@huanmeng-qwq
Copy link
Contributor Author

Thank you for the context! I completely understand the need to refactor the permission system for 4.x. Should we proceed with using dev.rollczi.litecommands.permission.PermissionResolver as the new standard moving forward? Happy to help test these changes if needed.

@Rollczi
Copy link
Owner

Rollczi commented Mar 2, 2025

I'm not sure about all the changes yet. They are not yet for testing. I'll try to finish it soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants