Skip to content

Conversation

theSarrum
Copy link
Contributor

When merged, this PR will:

  • Optimize bandwidth usage by using the client global variable instead of passing the player by parameter.
  • Fix permission checking by using an existing action name.

@Dezash
Copy link
Contributor

Dezash commented Apr 17, 2021

I think it would be better to add the takescreenshot permission to the default ACL configuration rather than remove it

@theSarrum
Copy link
Contributor Author

I think it would be better to add the takescreenshot permission to the default ACL configuration rather than remove it

I thought about it, but decided that it makes no sense to restrict screenshots for certain ACL groups, because they do not contain any private information.
Then there are 2 options:

  1. Add one permission (command.screenshots?) that will block full access to screenshots
  2. Add different permissions for different actions:
    2.1. 'Take New' - command.takescreenshot
    2.2. 'Delete' - command.deletescreenshot
    2.3. 'View' - command.viewscreenshot
    2.4. 'Refresh' - command.listscreenshots

@Dezash
Copy link
Contributor

Dezash commented Apr 17, 2021

More granular permissions are better in my opinion. You may not want trial moderators deleting screenshots or sometimes even taking them. However, the mentioned permissions should use "general" prefix instead of "command".

@theSarrum
Copy link
Contributor Author

theSarrum commented Apr 18, 2021

I agree with the prefix, but if we want to disable the buttons, there is no other option.

right = "command."..right

@Pirulax
Copy link
Contributor

Pirulax commented Apr 19, 2021

I think you should be able to change the trigger sources on client side to be localPlayer instead of resoureRoot, then just attach the handler to root on the server side.

@theSarrum
Copy link
Contributor Author

I have made changes, now we can restrict the rights for certain groups.

I think you should be able to change the trigger sources on client side to be localPlayer instead of resoureRoot, then just attach the handler to root on the server side.

Not sure if this is necessary. But if others agree, I'll do it.
Also weird that then we will have to attach to root on the server instead of resourceRoot.

@sacr1ficez
Copy link
Contributor

sacr1ficez commented May 4, 2021

Not sure if this is necessary. But if others agree, I'll do it.

Why not. Would be nice to follow this in any upcoming PR, if possible.

@theSarrum
Copy link
Contributor Author

Why not. Would be nice to follow this in any upcoming PR, if possible.

Done.

@jlillis jlillis merged commit 76b121f into multitheftauto:master Jun 7, 2021
@patrikjuvonen patrikjuvonen added this to the 1.5.9 milestone Apr 22, 2022
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.

6 participants