Skip to content

WIP Permissions System - #594

Merged
GhzGarage merged 12 commits into
qbcore-fivem:mainfrom
GhzGarage:main
Mar 30, 2022
Merged

WIP Permissions System#594
GhzGarage merged 12 commits into
qbcore-fivem:mainfrom
GhzGarage:main

Conversation

@GhzGarage

@GhzGarage GhzGarage commented Mar 28, 2022

Copy link
Copy Markdown
Contributor

This would replace the current permissions system and remove the database table by utilizing ace permissions

For example in your server.cfg:

add_ace group.join qbadmin.join allow # allow join server when closed

add_ace group.god god allow # allow god commands
add_principal group.god group.admin # also allow admin commands
add_principal group.god group.join # allow gods to join server when closed

add_ace group.admin admin allow # allow admin commands
add_principal group.admin group.mod # also allow mod commands

add_ace group.mod mod allow # allow mod commands

add_ace builtin.everyone user allow # allow everyone to use user commands

@GhzGarage GhzGarage self-assigned this Mar 28, 2022
@GhzGarage GhzGarage mentioned this pull request Mar 28, 2022
@CodeRedDev

Copy link
Copy Markdown

I just scrolled over it as I don't have the time right now to go through it in detail. One thing I found though is that PlayerData.permission is not set at all. Sure you can use QBCore.Functions.HasPermission() but in some cases it is nice to have the convenience member variable permission. E.g. a AdminDuty script I wrote yesterday relies on checking which permission group a player is in and checks that over PlayerData.permission.

I will look into it in detail when I get back home this evening. Thanks for the really fast work though ;)

@GhzGarage

Copy link
Copy Markdown
Contributor Author

Yeah permission player data was never a thing until introduced recently so no scripts from the GitHub use it, I can looking at adding it back though. Just seems useless because you just use the native checking ace instead of calling a function in the core

@CodeRedDev

CodeRedDev commented Mar 28, 2022

Copy link
Copy Markdown

Well it depends on the use-case. For a simple "Has-Permissions-Check" the native ace check does the trick but if you want to map something in the script to a specific group there is no native to get the string representation of this group (please correct me if I'm wrong).

Here some examples pseudo code (using permissionGroup instead of permission to be more precise):

RegisterCommand('setModerationOutfit', function(src, args)
    local permissionGroup = QBCore.Functions.GetPlayerData().permissionGroup
    local outfitData = Config.OutfitData[permissionGroup] -- getting the configured outfit data matching the players permission group

    ApplyPlayerOutfit(outfitData)
end)


function GetNotificationStringByGroup()
    local permissionGroup = QBCore.Functions.GetPlayerData().permissionGroup
    
    if permissionGroup == 'supporter'
         return 'A supporter is on the way to your current location. Please do not panic!'
    elseif ...
    -- [...]
end

@CodeRedDev

Copy link
Copy Markdown

I got 2 issue here:

@qb-core/server/commands.lua:21: attempt to call a nil value (global 'GetPlayer')
  1. After fixing the first on my local I tried the GetPermission() function and it always returns user. Unfortunately I think IsPlayerAceAllowed() only checks if a player has the permissions for a specific command. GetPermission() would need some kind of IsPlayerInPrincipal() function.

@GhzGarage

Copy link
Copy Markdown
Contributor Author

Yeah I figured out a better way, I’ll be adding that here

@CodeRedDev

Copy link
Copy Markdown

Oh and I just stumbled over another thing that is not working. The calls to HasPermission() also use the permission parameter that represents the group and not the ace / command.

@CodeRedDev

Copy link
Copy Markdown

I've tested your recent changes and they seem to work. Also because I finally understood your idea :D To add groups and players you would have to put the following into server.cfg , right?

add_ace group.mod mod allow
add_principal identifier.fivem:xxxxxxxxx group.mod 

@CodeRedDev

Copy link
Copy Markdown

I got a suggestion for QBCore.Commands.Add(). Instead of a string permission it could take a string table. That way it would be possible to configure commands to be called by multiple permission levels.

As this would break existing code single permission strings must be supported too. I wrote some code for this:

function QBCore.Commands.Add(name, help, arguments, argsrequired, callback, permission)
    local restricted = true -- Default to restricted for all commands
    if not permission then permission = 'user' end -- some commands don't pass permission level
    
    local isMultiLevel = false
    if type(permission) == 'table' then
        -- a permission string table was passed
        isMultiLevel = true
        for k, v in pairs(permission) do
            permission[k] = v:lower()
        end
    else
        -- if permission is not table it must be string
        permission = permission:lower()
    end
    
    if permission == 'user' then restricted = false end -- allow all users to use command

    RegisterCommand(name, callback, restricted) -- Register command within fivem

    if isMultiLevel then
        for _, perm in pairs(permission) do
            if not QBCore.Commands.IgnoreList[perm] then -- only create aces for extra perm levels
                ExecuteCommand(('add_ace group.%s command.%s allow'):format(perm, name))
            end
        end
    else
        if not QBCore.Commands.IgnoreList[permission] then -- only create aces for extra perm levels
            ExecuteCommand(('add_ace group.%s command.%s allow'):format(permission, name))
        end
    end

    if not isMultiLevel then permission = { permission } end -- save permission in table format

    QBCore.Commands.List[name:lower()] = {
        name = name:lower(),
        permission = permission,
        help = help,
        arguments = arguments,
        argsrequired = argsrequired,
        callback = callback
    }
end
function QBCore.Functions.HasPermission(source, permission)
    local src = source

    if type(permission) == 'table' then
        for _, perm in pairs(permission) do
            if IsPlayerAceAllowed(src, perm) then return true end
        end
    end

    if IsPlayerAceAllowed(src, permission) then return true end
    return false
end

@CodeRedDev

CodeRedDev commented Mar 30, 2022

Copy link
Copy Markdown

Also this does not seem to make a lot of sense:

https://github.com/qbcore-framework/qb-core/blob/48a73aff83ada6ea9f81668607a7784d70f8b483/server/events.lua#L206

I checked the whole QBCore Framework build for any command that sets the permission to a job name and have not found one. Every job script I saw that registers commands checks for the job name itself.

@GhzGarage

GhzGarage commented Mar 30, 2022

Copy link
Copy Markdown
Contributor Author

I've tested your recent changes and they seem to work. Also because I finally understood your idea :D To add groups and players you would have to put the following into server.cfg , right?

add_ace group.mod mod allow
add_principal identifier.fivem:xxxxxxxxx group.mod 
## Permissions

# Resources
add_ace resource.qb-core command allow # Allow qb-core to execute commands

# Gods
add_ace qbcore.god command allow # Allow all commands

# Inheritance
add_principal qbcore.admin qbcore.mod # Allow admins access to mod commands
add_principal identifier.license:xxxxx qbcore.xxx

@GhzGarage

Copy link
Copy Markdown
Contributor Author

Also this does not seem to make a lot of sense:

https://github.com/qbcore-framework/qb-core/blob/48a73aff83ada6ea9f81668607a7784d70f8b483/server/events.lua#L206

I checked the whole QBCore Framework build for any command that sets the permission to a job name and have not found one. Every job script I saw that registers commands checks for the job name itself.

Was probably some weird attempt at locking commands to jobs but with the new system you can just assign an ace to players with a certain job

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants