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

✨ feat: add setAccountSerial #3587

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

camargo2019
Copy link

@camargo2019 camargo2019 commented Jul 20, 2024

This PR adds a new function to edit the serial of a specific account. Currently, when players change their PC, the server owner needs to shut down the server and modify the internal.db file to update the player's serial. With this new function, this process is simplified, eliminating the need to shut down the server.

Example:

setAccountSerial(getPlayerAccount(thePlayer), "newSerial")
getPlayerAccount(thePlayer):setSerial("newSerial")

@camargo2019 camargo2019 marked this pull request as ready for review July 20, 2024 00:17
@camargo2019 camargo2019 marked this pull request as draft July 20, 2024 00:24
@camargo2019 camargo2019 marked this pull request as ready for review July 20, 2024 00:31
Server/mods/deathmatch/logic/luadefs/CLuaAccountDefs.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/luadefs/CLuaAccountDefs.h Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/luadefs/CLuaAccountDefs.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CStaticFunctionDefinitions.h Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CAccount.h Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CAccount.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/luadefs/CLuaAccountDefs.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/luadefs/CLuaAccountDefs.cpp Outdated Show resolved Hide resolved
@Nico8340
Copy link
Contributor

The basic concept of this pull request is understandable, but such a change must also be reviewed from a security point of view, as the modifiability of serials belonging to accounts carries many risks in my opinion.

@camargo2019
Copy link
Author

The basic concept of this pull request is understandable, but such a change must also be reviewed from a security point of view, as the modifiability of serials belonging to accounts carries many risks in my opinion.

So, I believe since the function is only on the server side, no problem

@camargo2019 camargo2019 requested a review from Nico8340 July 20, 2024 01:21
@TracerDS
Copy link
Contributor

This PR adds a new function to edit the serial of a specific account. Currently, when players change their PC, the server owner needs to shut down the server and modify the internal.db file to update the player's serial. With this new function, this process is simplified, eliminating the need to shut down the server.

Example:

setAccountSerial("userName", "newSerial")

What would be the benefits of that function?

Server/mods/deathmatch/logic/luadefs/CLuaAccountDefs.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/luadefs/CLuaAccountDefs.h Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CAccount.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CAccount.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CAccount.h Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CStaticFunctionDefinitions.h Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/luadefs/CLuaAccountDefs.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/luadefs/CLuaAccountDefs.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/luadefs/CLuaAccountDefs.h Outdated Show resolved Hide resolved
@xLive

This comment was marked as outdated.

@camargo2019
Copy link
Author

This PR adds a new function to edit the serial of a specific account. Currently, when players change their PC, the server owner needs to shut down the server and modify the internal.db file to update the player's serial. With this new function, this process is simplified, eliminating the need to shut down the server.
Example:

setAccountSerial("userName", "newSerial")

What would be the benefits of that function?

I believe it would be easier to change the Accounts serial in internal.db

Imagine a server that has 500 players online, and someone from the administration changed computers due to an emergency, they would have to shut down the server just to change its serial number! With this function, we eliminate this need

@camargo2019
Copy link
Author

Is there any particular reason why this function uses an account name instead of an account element?

I tried following the setAccountPassword and setAccountName pattern
But I don't have specific reasons.

@camargo2019 camargo2019 requested review from Nico8340 and TracerDS July 20, 2024 12:52
@TracerDS
Copy link
Contributor

I believe it would be easier to change the Accounts serial in internal.db

Imagine a server that has 500 players online, and someone from the administration changed computers due to an emergency, they would have to shut down the server just to change its serial number! With this function, we eliminate this need

Why would they need to shut down the server? You can login into the account from 5 different computers without issues.
There isnt one serial per account. Accounts just store the last serial that logged in.
You can store the original serial via setAccountData just fine

@camargo2019
Copy link
Author

Why would they need to shut down the server? You can login into the account from 5 different computers without issues.
There isnt one serial per account. Accounts just store the last serial that logged in.
You can store the original serial via setAccountData just fine

Let me try to give a better example. I don't use ElementData and I only use the Multi Theft Auto login system. I create a simple if getAccountSerial(account) == getPlayerSerial(player) which, in theory, would get the last serial the player connected with and validate it with the current serial, right? But now everyone who is connecting to my server is passing this validation, and the authserial <account> removelist is not intended to remove this serial. What should I do in this case? Should I temporarily remove the validation and restart my system? Or should I create my own serial management using ElementData or a database? Or, in this new case, should I use this function to update the serial and use the MTA's own structure?

The idea of bringing this function is just to make things easier

@TracerDS
Copy link
Contributor

Let me try to give a better example. I don't use ElementData and I only use the Multi Theft Auto login system. I create a simple if getAccountSerial(account) == getPlayerSerial(player) which, in theory, would get the last serial the player connected with and validate it with the current serial, right? But now everyone who is connecting to my server is passing this validation, and the authserial <account> removelist is not intended to remove this serial. What should I do in this case? Should I temporarily remove the validation and restart my system? Or should I create my own serial management using ElementData or a database? Or, in this new case, should I use this function to update the serial and use the MTA's own structure?

The idea of bringing this function is just to make things easier

You should never rely on just serial as your authentication as they are not reliable.
authserial ... removelast will remove the last logged serial from the account. If everyone who is connecting to your server is passing the if getAccountSerial(account) == getPlayerSerial(player) then that means that everyone has the same serial.

You should connect several identification methods (ip, serial, specs, etc.) together to in some way identify a player.
This function will bring nothing but chaos to mta imo

@camargo2019
Copy link
Author

camargo2019 commented Jul 20, 2024

Let me try to give a better example. I don't use ElementData and I only use the Multi Theft Auto login system. I create a simple if getAccountSerial(account) == getPlayerSerial(player) which, in theory, would get the last serial the player connected with and validate it with the current serial, right? But now everyone who is connecting to my server is passing this validation, and the authserial <account> removelist is not intended to remove this serial. What should I do in this case? Should I temporarily remove the validation and restart my system? Or should I create my own serial management using ElementData or a database? Or, in this new case, should I use this function to update the serial and use the MTA's own structure?
The idea of bringing this function is just to make things easier

You should never rely on just serial as your authentication as they are not reliable. authserial ... removelast will remove the last logged serial from the account. If everyone who is connecting to your server is passing the if getAccountSerial(account) == getPlayerSerial(player) then that means that everyone has the same serial.

You should connect several identification methods (ip, serial, specs, etc.) together to in some way identify a player. This function will bring nothing but chaos to mta imo

Why do you think this will bring more chaos to MTA? Nowadays, server owners already perform this action, but most often manually through the internal.db. Shops usually carry out this type of validation, so what we can do to increase security is to prevent malicious scripts by adding appropriate rules to the acl.xml.

And also, removelist only removes the serials from the serialusage table, not from accounts. The getAccountSerial function retrieves the serial from accounts.

@TracerDS
Copy link
Contributor

Why do you think this will bring more chaos to MTA? Nowadays, server owners already perform this action, but most often manually through the internal.db. Shops usually carry out this type of validation, so what we can do to increase security is to prevent malicious scripts by adding appropriate rules to the acl.xml.

acl.xml have no effect on client side functions and events. It only prevents server side code from doing stuff.
I see no reason why this function should be added to mta. deleteAccountLastSerial for clearing last serial would be good.
Setting it manually is not good

@camargo2019
Copy link
Author

acl.xml have no effect on client side functions and events. It only prevents server side code from doing stuff.
I see no reason why this function should be added to mta. deleteAccountLastSerial for clearing last serial would be good.
Setting it manually is not good

But this function is on the server side

@TracerDS
Copy link
Contributor

But this function is on the server side

I understand that. I was responding to

what we can do to increase security is to prevent malicious scripts by adding appropriate rules to the acl.xml.

If you have backdoors in your scripts (or download malware scripts to your server) then no matter what functions you have at your disposal, its useless.

@camargo2019
Copy link
Author

But this function is on the server side

I understand that. I was responding to

what we can do to increase security is to prevent malicious scripts by adding appropriate rules to the acl.xml.

If you have backdoors in your scripts (or download malware scripts to your server) then no matter what functions you have at your disposal, its useless.

So you think it’s better to delete the serial rather than setting a new one? By modifying the function to deleteAccountLastSerial?

@jlillis
Copy link

jlillis commented Jul 20, 2024

when players change their PC, the server owner needs to shut down the server and modify the internal.db file to update the player's serial.

I assume by players, you mean admins and members of other ACL groups you have enabled authserial protection for? You should be able to get them access by following the instructions on the wiki. Adding a new function for that purpose should not be necessary.

This function would still be useful if you're just looking to update the serial returned by getAccountSerial. It does raise another question - should getAccountSerial be returning the serial the account was registered with or the last serial used to log in? Maybe out of scope for this discussion.

If you have backdoors in your scripts (or download malware scripts to your server) then no matter what functions you have at your disposal, its useless.

Script backdoors are not a threat that the authserial system is meant to address. The ACL system will mitigate some of that threat, but it is ultimately the responsibility of the server owner to manage. Protecting this function in the same way as setAccountPassword is sufficient.

@camargo2019
Copy link
Author

This function would still be useful if you're just looking to update the serial returned by getAccountSerial. It does raise another question - should getAccountSerial be returning the serial the account was registered with or the last serial used to log in? Maybe out of scope for this discussion.

This function does exactly that: it updates the serial from getAccountSerial

@Lpsd
Copy link
Member

Lpsd commented Jul 29, 2024

Can the acl right be put in the same group as the other setAccount* functions?

@camargo2019
Copy link
Author

@Lpsd Do you say put it in the Moderator?

@Nico8340
Copy link
Contributor

@Lpsd Do you say put it in the Moderator?

I think so
If you look at the other entries, you can see that they also belong to the moderator group

@CrosRoad95
Copy link

Why not just add option to have multiple serials allow to login to the same account?

@camargo2019
Copy link
Author

@Nico8340 adjusted

@CrosRoad95 The idea is to update the serial from getAccountSerial and not from authserial

@CrosRoad95
Copy link

Also, if you add setAccountSerial function you in practice introducing a way to bypass already existing protections EVEN you updated acl, why you may guess? People don't updating, reusing resources, and in this resources often lies outdated acl.xml.

Good example is dgs, even thisdp fixed vulnerability there YEARS ago, it is still present on many, many servers

@camargo2019
Copy link
Author

camargo2019 commented Sep 30, 2024

@CrosRoad95 I agree with you; I believe that ACL is not the most secure resource. However, the function resides on the server side, so it is the total responsibility of whoever is setting up their server to be aware of and consent to the use of this function

We should not be held responsible for the misuse of certain functions, such as debugHook, which many people are using nowadays to break the protections of resource stores. We do not take action on this because it is not within our jurisdiction nor is it our responsibility to address the misuse of these functions.

Remembering that this is just an example, the case mentioned has nothing to do with this function.

@Fernando-A-Rocha
Copy link
Contributor

Fernando-A-Rocha commented Nov 6, 2024

getAccountSerial returns the last serial that logged onto the specified account.

In my opinion, you shouldn't use a function to alter this value. It's automatic...

Instead, use setAccountData or whatever for security and serial validation purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants