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

Added Persistent Ace-Hybrid Permissions via SQL #1118

Closed
wants to merge 3 commits into from

Conversation

nick-perry14
Copy link

Description

The PR adds back persistent permissions via SQL. This allows the qb-adminmenu assignment to work like it used to, and the /addpermission is no longer an ephemeral command.

This PR seeks to be the "best of both worlds" between the legacy permission check that always checked the DB, and the current system that requires CFG file changes with ace or dealing with ephemerality.

Resolves #888

Checklist

  • I have personally loaded this code into an updated qbcore project and checked all of its functionality.
  • My code fits the style guidelines.
  • My PR fits the contribution guidelines.

Copy link
Contributor

@ChristianBDev ChristianBDev left a comment

Choose a reason for hiding this comment

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

Tested LGTM, would be a nice feature to add ModifyPermission function to delete the data from the table to change just the permission

@GhzGarage GhzGarage self-assigned this Jul 10, 2024
@Mycroft-Studios
Copy link
Contributor

Mycroft-Studios commented Jul 12, 2024

An issue i can see here, is that any ace permissions granted via the server cfg, or another resource (ox lib as an example) wont be accounted for.
Another thing here, is that the general logic overall, doesnt really work with ace permissions, which seems to stem from the admin menu itself.
ace perms are inheritance base, where you have your groups (principals) and the permissions that they hold (the aces).
the thing here being, a player can hold both deny and allow ace at the same time,
so lets say, you give someone 2 principals, the first has an allow, and then second has a deny on the same ace, well, then suddenly,
they have access... but then they also... dont have access to it. Usually, this wouldnt occur, because well, the groups inherit eachother.
you start with your base principal, say what that can do,
you then have another, which inherits what the base one can do, and you give this one, more access
and then keep going

However, i like the idea, and i think theres a variation of this solution that could work for all cases

@nick-perry14
Copy link
Author

nick-perry14 commented Jul 12, 2024

@Mycroft-Studios What this does is it creates a system that allows dynamically assigned ace permissions. If there's another permission created by a resource (that you would like to grant to this specific user) you would have to add said permission to the qbcore group. Once that happens then it doesn't really matter.

This PR attempts to create a hybrid and dynamic solution where you would NOT need to add the user anywhere in the cfg and their ACE group would be assigned upon joining. This would allow "on-the-fly" ace permission changes to occur.

As for:

Another thing here, is that the general logic overall, doesnt really work with ace permissions, which seems to stem from the admin menu itself.

The whole point of this PR is to dynamically assign ace permission GROUPS so I don't know what this means.

so lets say, you give someone 2 principals, the first has an allow, and then second has a deny on the same ace, well, then suddenly,
they have access... but then they also... dont have access to it. Usually, this wouldnt occur, because well, the groups inherit eachother

Unsure what this means, but either FiveM is a permissive or prohibitive environment. This means that either an explicit allow overrides an explicit deny, or vice versa (or the third option, where the LAST ace assigned is the one kept). Either way would be irrelevant to this PR

you start with your base principal, say what that can do,
you then have another, which inherits what the base one can do, and you give this one, more access
and then keep going

Unsure of what this means too.

@Mycroft-Studios
Copy link
Contributor

@Mycroft-Studios What this does is it creates a system that allows dynamically assigned ace permissions. If there's another permission created by a resource (that you would like to grant to this specific user) you would have to add said permission to the qbcore group. Once that happens then it doesn't really matter.

This PR attempts to create a hybrid and dynamic solution where you would NOT need to add the user anywhere in the cfg and their ACE group would be assigned upon joining. This would allow "on-the-fly" ace permission changes to occur.

As for:

Another thing here, is that the general logic overall, doesnt really work with ace permissions, which seems to stem from the admin menu itself.

The whole point of this PR is to dynamically assign ace permission GROUPS so I don't know what this means.

so lets say, you give someone 2 principals, the first has an allow, and then second has a deny on the same ace, well, then suddenly,
they have access... but then they also... dont have access to it. Usually, this wouldnt occur, because well, the groups inherit eachother

Unsure what this means, but either FiveM is a permissive or prohibitive environment. This means that either an explicit allow overrides an explicit deny, or vice versa (or the third option, where the LAST ace assigned is the one kept). Either way would be irrelevant to this PR

you start with your base principal, say what that can do,
you then have another, which inherits what the base one can do, and you give this one, more access
and then keep going

Unsure of what this means too.

Just saw that you have a PR to the admin menu that goes along with it,
both together should fix any concerns i have

Copy link

This PR has had 60 days of inactivity & will close within 7 days

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

Successfully merging this pull request may close these issues.

[SUGGESTION] Add command for permanent permissions
4 participants