search

AppleDash / SaneEconomy

Finally, a sane economy plugin for Bukkit.
https://www.spigotmc.org/resources/saneeconomy-simple-but-featureful-economy.26223/
GNU General Public License v3.0
19 stars 24 forks source link

Improvement: SQL Permission Checks #93

Closed deploring closed 4 years ago

deploring commented 4 years ago

G'day, Me again. I recently decided to tighten the permissions that my production database user has because best practices and whatnot. Specifically, my user only has permissions to manage the creation and maintenance of tables, along with full read/write access to them. Unfortunately, this led me to chase my tail for a while and I had to step through your database logic.

I've received an error message stating:

Caused by: com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Access denied for user 'xxx'@'x.x.x.x' to database 'xxx'

This is because the plugin attempts to lock the balance table when setting a balance, which my production user does not have permission to do. I consider locking tables to be an administrative operation. I do not fully know what you are trying to achieve with the write lock.

I believe you are trying to make sure that the balance changes are applied in order because some balance updates happen sync while some happen async. I don't see any concurrency implications though. Would you mind explaining its purpose? Has there ever been an instance where players have lost money due to two calls of setBalance being committed out of order?

Anyways, my request is that you perform a preliminary table lock after creating the tables to check that the plugin has permission to lock them. That way you can catch the error and properly inform the user that they do not have table lock privileges. It would definitely help out people who are not going to naively hand out SQL permissions, but then again I would consider those kind of people equally as rare as this situation.

Love your work! Cheers.

AppleDash commented 4 years ago

The purpose for the write lock is so that in the case of multiple servers accessing the same database, only one server can mess with the economy tables at once.

I can add that check soon.

deploring commented 4 years ago

I see. What is the issue being caused from multiple connections accessing the economy tables at the same time? Have you tried using transactions?

Awesome, glad to hear. Will help a few people in the future. 👍

AppleDash commented 4 years ago

Still have this in my mind. I am not 100% sure about the issue that was caused, I just remember there WAS an issue and this fixed it. Transactions could be a workaround.

AppleDash commented 4 years ago

Should be fixed in the master branch, and a new version will be released soon.