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

Permissions API support concept 2 #1864

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

altrisi
Copy link
Collaborator

@altrisi altrisi commented Jan 19, 2024

Prototype on supporting the fabric permissions API for Carpet commands.
It'd only be used if the permission manager is present, so it's not required/included otherwise.

TBD:

  • Is this the way to do it? Or allow choosing the node in the rule/app? (like this old prototype) The other way is more intrusive IMO, it changes behaviour of a lot of things depending on whether another mod is present
  • Should we disable/hide the command rules if a permission api is present? It could make sense: if it's there, who installed Carpet will probably want to use permissions for commands. We could use a simple @Rule.Condition for that
  • (if no to the above) Is it fine to put the hint message in the general command validator? Extensions may not support permissions yet (unlike the above method, this doesn't just automatically work for them, they have to call a different method)

TODO:

  • Hook it with Scarpet apps, their logic is more complex so I didn't hook it yet for the concept
  • Check that we're doing fallbacks correctly, we may need to pass a fallback to the api.
  • Add actual api call
  • Ensure the api can't be present without a permission handler mod, or see if we can check that to disable this system in that case

Maybe future enhancements:

  • Check if there's a way we can hook to give a different permissions group/whatever it's called to scarpet run, so owners can configure what apps can run
  • Maybe add subcommand specific permissions for scarpet apps? Maybe for a followup, it'd also be nice to have without perms

Note that I don't know a lot about the permissions api stuff, barely used it a similar one many years ago.

Fixes #1850.

TBD: Not convinced to add it here, given extension commands may not support it
This will have to be pending the Scarpet command refactor thing to not
have conflicts
Hooking to Scarpet app commands is a bit more complex
@altrisi altrisi added the mod compat Compatibility with other mods label Jan 19, 2024
@sakura-ryoko
Copy link

sakura-ryoko commented Jan 20, 2024

Hey, nice work. I just wanted to mention that there are additional versions of fabric-permissions-api that are fairly common to use. Take a look at implementing a check for Drex's fork, as he is implementing it in a bunch of popular mods that he contributes code for.
https://github.com/DrexHD/fabric-permissions-api
This fork keeps the same name, but bumps the version to 0.3 .

@altrisi
Copy link
Collaborator Author

altrisi commented Jan 26, 2024

As far as I know that 0.3 is compatible with mods depending on the original, so given Carpet wouldn't actually include the API (just use it if it's there), the idea is it'd be compatible with either out of the box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod compat Compatibility with other mods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support fabric permissions api for better customizing carpet commands
2 participants