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

0009626: Added reloadacl command for server console #131

Merged
merged 2 commits into from Feb 15, 2018

Conversation

Projects
8 participants
@Timic3
Contributor

Timic3 commented May 11, 2017

This simple pull request adds reloadacl server command. See #9626

@qaisjp qaisjp added the enhancement label May 13, 2017

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp May 13, 2017

Member

Hey @Timic3! Thank you for your pull request.

The code looks great (although I am concerned about whether only administrators can run the command...), but I think it would be best to actually add this command to the admin resource.

Would you like to work on that? I'm happy to assign mantis 9626 to you!

Member

qaisjp commented May 13, 2017

Hey @Timic3! Thank you for your pull request.

The code looks great (although I am concerned about whether only administrators can run the command...), but I think it would be best to actually add this command to the admin resource.

Would you like to work on that? I'm happy to assign mantis 9626 to you!

@Timic3

This comment has been minimized.

Show comment
Hide comment
@Timic3

Timic3 May 14, 2017

Contributor

Hi @qaisjp, the main reason I added this is because when people start the server for the first time, they don't edit the ACL (at least I don't) and to add themselves to Admin ACL group, they'd need to restart the server. If this would be added to the admin resource, will console still be able to use /reloadacl? And if people didn't download the resources package, they wouldn't be able to use this command.

I think players cannot use this command (correct me if I am wrong), because I did this only for server console.

Contributor

Timic3 commented May 14, 2017

Hi @qaisjp, the main reason I added this is because when people start the server for the first time, they don't edit the ACL (at least I don't) and to add themselves to Admin ACL group, they'd need to restart the server. If this would be added to the admin resource, will console still be able to use /reloadacl? And if people didn't download the resources package, they wouldn't be able to use this command.

I think players cannot use this command (correct me if I am wrong), because I did this only for server console.

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash May 14, 2017

Contributor

Console has Admin permissions by default so they would be able to use it.

Contributor

Dezash commented May 14, 2017

Console has Admin permissions by default so they would be able to use it.

@Timic3

This comment has been minimized.

Show comment
Hide comment
@Timic3

Timic3 May 14, 2017

Contributor

Still, that doesn't eliminate the problem where they haven't downloaded the resources.

Contributor

Timic3 commented May 14, 2017

Still, that doesn't eliminate the problem where they haven't downloaded the resources.

@4O4

This comment has been minimized.

Show comment
Hide comment
@4O4

4O4 May 14, 2017

Contributor

I agree with @Timic3. ACL is built-in MTA feature, which is tightly coupled with the xml config file. IMO there should also be a built-in command for such trivial task as reloading this config, which eliminates the need to install / code by hand any additional resources.

Contributor

4O4 commented May 14, 2017

I agree with @Timic3. ACL is built-in MTA feature, which is tightly coupled with the xml config file. IMO there should also be a built-in command for such trivial task as reloading this config, which eliminates the need to install / code by hand any additional resources.

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp May 14, 2017

Member

These are reasonable arguments.

Also, to answer your question, the console can execute normal addCommandHandler commands.

Thanks again for your pull request!

Member

qaisjp commented May 14, 2017

These are reasonable arguments.

Also, to answer your question, the console can execute normal addCommandHandler commands.

Thanks again for your pull request!

@qaisjp

qaisjp approved these changes May 14, 2017

@ccw808

This comment has been minimized.

Show comment
Hide comment
@ccw808

ccw808 May 14, 2017

Member

I think the command needs to block anyone from using it. See the example at the start of CConsoleCommands::AuthSerial

Member

ccw808 commented May 14, 2017

I think the command needs to block anyone from using it. See the example at the start of CConsoleCommands::AuthSerial

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp May 14, 2017

Member

I'm not sure... the start command doesn't seem to have the block. See here.

Member

qaisjp commented May 14, 2017

I'm not sure... the start command doesn't seem to have the block. See here.

@ccw808

This comment has been minimized.

Show comment
Hide comment
@ccw808

ccw808 May 14, 2017

Member

start command is protected in acl.xml

Member

ccw808 commented May 14, 2017

start command is protected in acl.xml

@@ -1827,6 +1827,17 @@ bool CConsoleCommands::AuthorizeSerial( CConsole* pConsole, const char* szArgume
return false;
}
bool CConsoleCommands::ReloadAcl(CConsole* pConsole, const char* szArguments, CClient* pClient, CClient* pEchoClient)
{

This comment has been minimized.

@qaisjp

qaisjp May 14, 2017

Member

Needs to make sure only console / people with rights can use this command. See this example.

@qaisjp

qaisjp May 14, 2017

Member

Needs to make sure only console / people with rights can use this command. See this example.

@Timic3

This comment has been minimized.

Show comment
Hide comment
@Timic3

Timic3 May 14, 2017

Contributor

Okay, I added that check you requested - I also added the command to ACL, so only users in Admin group can execute it.

Contributor

Timic3 commented May 14, 2017

Okay, I added that check you requested - I also added the command to ACL, so only users in Admin group can execute it.

@qaisjp

qaisjp approved these changes May 14, 2017

@ccw808 ccw808 self-requested a review May 15, 2017

@ccw808

ccw808 approved these changes May 15, 2017

@Timic3

This comment has been minimized.

Show comment
Hide comment
@Timic3

Timic3 Aug 2, 2017

Contributor

I tested everything (reloading ACL with no rights, with rights & server console) and as far as I know, everything works. I think this can be merged, but if you want to test it again just to be sure, go ahead.

Contributor

Timic3 commented Aug 2, 2017

I tested everything (reloading ACL with no rights, with rights & server console) and as far as I know, everything works. I think this can be merged, but if you want to test it again just to be sure, go ahead.

@Necktrox Necktrox merged commit ecb5d14 into multitheftauto:master Feb 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@patrikjuvonen patrikjuvonen added this to In progress in release/v1.5.6 via automation Aug 7, 2018

@patrikjuvonen patrikjuvonen added this to the 1.5.6 milestone Aug 7, 2018

@patrikjuvonen patrikjuvonen moved this from In progress to Done in release/v1.5.6 Aug 7, 2018

@patrikjuvonen patrikjuvonen changed the title from Added reloadacl command for server console (#9626) to 0009626: Added reloadacl command for server console Sep 4, 2018

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