Differentiate Floodgate players in database

[Smart123s]

Summary of your change

Please read #700.
Added a new column to the database called Floodgate.
This will make the life of Bedrock and Java players with the same name a lot easier.

Why Draft

I've just started working on this, and I wanted to share the code while I'm progressing with it.
At this stage you have to delete the old database, or the server will crash when someone tries to join it.

Todo

• Database migrator (so the old database won't have to be deleted)
• Migrate every part of the code to be aware of the new Floodgate field in the database
  • Migrate activate plugin message*
  • Migrate /cracked and /premium commands (self)*
    • Fix issue described at "Floodgate detection" (issue Setting floodgatePrefixWorkaround to true gives "Your username contains illegal characters" to Bedrock Clients #689)
  • Migrate /cracked and /premium commands (others)*
• Check what the Premium field in the db does for Floodgate players (so check for leftover code)
• Debug why uuid and lastIp are null for Floodgate players in the database (code)

*No longer needed, becuase usernames are now unique (#709 (comment)).

To Test

just notes for myself, so that I won't forget it

• Server without Floodgate
• BungeeCord
• Premium/Cracked commands
• AsyncToggleMessage
• Auto login/register with same name Java and Bedrock accounts
  Result: only works on the platform where premium login was enabled

Floodgate detection

#689 needs to be fixed. (I'm not linking that issue to his PR on purpose, as it's not fixed yet.)
In short, if ProtocolLib prefix workaround is enabled, the following code wont work, after the prefix is added:
https://github.com/games647/FastLogin/blob/9b04ea5c89e0b99bc273d28a1d9b0483c0a1b903/core/src/main/java/com/github/games647/fastlogin/core/hooks/bedrock/FloodgateService.java#L105-L114

Related issue

Fixes #700
Fixes #696
Fixes #633

[Smart123s]

@games647 I've got some questions.

As of now, (on branch main/9b04ea5), FastLogin allows Floodgate and Java players to be registered with the same name (without prefixes). However, the database contains

https://github.com/games647/FastLogin/blob/af2df500577db8a01c9cf664477fa11921eb5e97/core/src/main/java/com/github/games647/fastlogin/core/storage/SQLStorage.java#L59-L60

which forbids registering two players with the same name.

While I am strongly against registering multiple players with the same name, because it can cause confusion with some plugins (especially auth plugins), but there are folks out there already using this approach. For example, see #696.

I have came up with the following possibilities as a solution:

• Deny joining the server, if the name has been registered (=saved in db) from a different platform (=Floodgate/Java)
  • This would be a bit forcing in my opinion, considering that Floodgate allows that (via disabling username prefixes)
• Allow auto login/registering only for the platform the player (name) joined first
  • With the approach, the player from second platform doesn't have to be saved in the db
• Remove the UNIQUE constraint from the db table, and allow same named registrations from multiple platforms (as it was before)
  • Are there any checks that should be introduced in other parts of the code, if the UNIQUE constraint is removed?
  • In this case, a non-registered option could be added to allowFloodgateNameConflict

[TuxCoding]

To be honest name changing is difficult topic. I made the UNIQUE decision earlier, because we try to find the player profile on login based on the player name. If there are two, who should we pick? Allowing name changes would mean the old name is longer up to date and should be removed, therefore unique.

Correct me if I'm wrong, but the referenced ticket is about the login with the same player name, that is the same account owner? The best option would be to allow both, because Floodgate authentication is identical to onlinemode verification. It only differs that FastLogin doesn't perform the validation. However, I noticed, this should only apply to linked profiles. Otherwise it should be forbidden for the second platform.

Although it would be possible to have two accounts with the same player name, because as far as I know Floodgate uses different UUID. Nevertheless it could cause conflicts like you mentioned.

[Smart123s]

I've decided to drop support for conflicting names alltogether. If a player is registered in Java, and logs in via Bedrock, they won't have access to auto login/register (they won't be kicked, so if they have the password, they can still log in). My reasons for dropping this "feature":

1. It's recommended to use floodgate prefixes, so ther can't be conflicting names.
2. If the same person owns the accounts on bedrock and java, they can simply link it.
3. If the name has a different owner on java and bedrock, then it's extremely unsafe, and it's not worth supporting. Two people shouldn't use the same name.
   If someone want's to support conflicting names, I won't stop them.

Also, sorry for being inactive lately, I will try to contribute more in the future.

[Smart123s]

Note for myslef: 87b21fd (and it's parents) have merge conflicts resolved with 6beaf19. This branch is currently based on 62d7320, as I couldn't strat up a server with anything newer. No longer relevant.

[Smart123s]

This pull request is more or less ready for review. I'll finish the remaining TODOs, run some test (done), and probably do a rebase.

Commits 1c63e70, 25e71e8, ae9772c, 24d552d, eab6ef9 could have been a separate pull request (I can still open a new one, if you'd like me to), but I thought that they fix an important issue (the floodgate prefix bug with ProtocolLib) that it should be fixed before the new database columns get added.

I might replace the getBedrockPlayer() function with the channel extraction method from 1c63e70 and ae9772c, but that could be done in a future PR. It'd also require me to figure out how to extract the player's channel in Bungee and in ProtocolSupport (and Velocity in the future), which would take some time.

For migrating legacy databases I came up with the idea to add a new integer column called DBVersion. If the database will have to be updated again in the future, we could simply increase the value of DBVersion. Is this approach OK?

---

Do you have any idea on how to make this line shorter?

https://github.com/games647/FastLogin/blob/ae9772c0b028c47367961eb000f3ab50e39cf940/bukkit/src/main/java/com/github/games647/fastlogin/bukkit/listener/protocollib/ProtocolLibListener.java#L295

This didn't help: https://stackoverflow.com/questions/41056517/breaking-a-long-url-to-several-line-in-javadoc

[TuxCoding]

For migrating legacy databases I came up with the idea to add a new integer column called DBVersion. If the database will have to be updated again in the future, we could simply increase the value of DBVersion. Is this approach OK?

Well there also the approach of doing migrations. This what I heard is normally used when doing schema changes. However I don't know if they fit here, because we are doing a decision during the login process and not on the data itself.

Do you have any idea on how to make this line shorter?

Well you can split the a-tag, but that's it.

<a href="">
lorem ipsum
</a>

Attribute href could also be on the next line, but that just looks strange.

[Smart123s]

Well there also the approach of doing migrations. This what I heard is normally used when doing schema changes.

The pluign you provided uses PHP, and I don't think that it'd fit for a java project. However, Flyway could be used, which has a Java API.

However I don't know if they fit here, because we are doing a decision during the login process and not on the data itself.

There are three approaches I could think of:

1. (current) Have a DBVersion column to check if the player was migrated or not.

• tbh, I'm starting to think that this is just unnecessary bloat

2. Set Floodgate as a nullable boolean, and use that instead of a DBVersion column

• and maybe make it nullable if a new database is created (so it's not an old one being migrated), although that would fragment the db structure, which could cause headaches in the future

3. Keep Floodgate as not null and tell everyone who updates the plugin that Floodgate players have to log in using their passwords, and type /premium to migrate themselves.

• this would be the best idea in the long term
• Floodgate support is still experimental, it's written 5 times in the config (1, 2, 3, 4, 5), so I could use it as an excuse to tell people that I don't care if a Floodgate player hasn't written down their automatically registered password. Although I'm pretty sure there would be a lot of new issues opened, and lots of people would kindly ask me to go to hell.

Which one should I go with? I'd actually prefer the second one. Regardless, I could use Flyway to add the missing column(s) to the database. Would that plugin/library be okay? EDIT: It might be a bit overkill for adding a single column. And most of the examples migrate the database via maven commands, so I think that the plugin is oriented towards codebases where the developer also maintains the database themselves. Also, is there any way I could properly integrate migrations into a Java project?

[TuxCoding]

I rather was talking about the concept of having change based migrations. So one table which is used to keep track of the applied migrations and then seperate files for each migration. The implementation can be very simple, because the files can fully be read and then applied. So we then have something like:

migrations/01_base_table.sql (the current table for existing setups)
migrations/02_add_floodgate_differentiation.sql
migrations/03_track_name_changes.sql (example)
migrations/04_drop_something.sql (example)

I don't think we need a library for that. We check the number of applied migrations and then apply the sql file(s) of which are missing.

What do you think about that? This might be a bit overkill, because it's often used in combinations with many contributors (then also with timestamps) and frequent scheme changes.

and maybe make it nullable if a new database is created (so it's not an old one being migrated), although that would fragment the db structure, which could cause headaches in the future

I guess that's the best option to migrate the old data while using the new structure for new players. We could later drop the backwards compatibility by migrating all nullable values to false.

[TuxCoding]

Looks good so far

[Smart123s]

I rather was talking about the concept of having change based migrations.

Then I misunderstood you, I thought you were talking about the library.

I don't think we need a library for that.

Me neither.

This might be a bit overkill

It'd make code cleaner, and it'd also make potential future migrations easier, so I think it's fine.

I guess that's the best option to migrate the old data while using the new structure for new players. We could later drop the backwards compatibility by migrating all nullable values to false.

This will be the way then.

[Smart123s]

I've moved the ProtocolLib fix to #876 so I won't have to rebase ProtocolLib related commits when working on the database. As long as the fix gets merged before the database modifications, things should be fine.

I'll remove the commits from this branch in a moment. I'll just have dinner beforehand. 🙃

[TuxCoding]

Looks very good so far.

[Smart123s]

I have tested the migration from 7425df8 (branch main) to 1d0e089 (#709) on paper 1.19.2 with SQLite and MariaDB and encountered no issues with it.

EDIT: I've found a bug.

[Smart123s]

@games647 The code is ready for review (and merge?), when you have some spare time. No need to rush. I think I've fixed everything I've noticed in my previous tests.

[Smart123s]

Hi, I know it's been a long time, but I kind of want to rewrite some parts.

While I remember that we had discussions about the Migrations table, and wether or not it's neccesarry. I've looked at the code of AuthMe, and they don't use a Migrations table. Here's what they use:

https://github.com/AuthMe/AuthMeReloaded/blob/master/src/main/java/fr/xephi/authme/datasource/MySQL.java#L191-L301
https://github.com/AuthMe/AuthMeReloaded/blob/master/src/main/java/fr/xephi/authme/datasource/MySqlMigrater.java

Do you think something similar would be sufficient here? edit: it'd simplify the code a lot

The commit history could also take some squashing here and there. (for example 2c515e6) And I'll also sign the commits you've pushed.

I saw that there's an integration-test-containers branch. Either that should be completed, or at least some JUnit tests should be added to make sure that database migrations won't break anytime soon.

edit: and there are some parts that I just generally don't like how I've approached them.

[TuxCoding]

The commit history could also take some squashing here and there. (and I'll also sign the commits you've pushed).

Warning I lost some commits during rebase. Maybe you could restore your version or I'll try to dig into my reflog.

EDIT: I think I found it.

Do you think something similar would be sufficient here?

To be honest: This migration concept could be a little too much for a single database change (atm). The idea of migrations comes from my experience with web projects where I saw it quite often with big benefits for multiple contributors (separating logical changes to different files) and being able to rollback. In the end, I'm fine with the AuthMe system too.

I saw that there's an integration-test-containers branch.

Yeah the idea was to test core functionality against a full Minecraft server. However, this is very hard to setup and optimize. Nevertheless, it could be very helpful for verifying aspects like login support.

Regarding the database system, this could be a lot easier and a good idea. By separating the encapsulating the database implementation from the Bukkit API we could be fine with only running a MySQL/MariaDB image.

[Smart123s]

Aside from the migrations part, I've changed only this:

diff --git a/core/src/main/java/com/github/games647/fastlogin/core/shared/FloodgateManagement.java b/core/src/main/java/com/github/games647/fastlogin/core/shared/FloodgateManagement.java
index 2f2922f5..c01c1478 100644
--- a/core/src/main/java/com/github/games647/fastlogin/core/shared/FloodgateManagement.java
+++ b/core/src/main/java/com/github/games647/fastlogin/core/shared/FloodgateManagement.java
@@ -152,16 +152,12 @@ public abstract class FloodgateManagement<P extends C, C, L extends LoginSession
             }
         }
 
-        if (!isRegistered && !isAutoAuthAllowed(autoRegisterFloodgate)) {
+        if ((!isRegistered && !isAutoAuthAllowed(autoRegisterFloodgate))
+                || (profile.getFloodgate() == FloodgateState.LINKED && !isLinked)
+                || (profile.getFloodgate() == FloodgateState.TRUE && isLinked)) {
             return;
         }
 
-        //logging in from bedrock for a second time threw an error with UUID
-        if (profile == null) {
-            profile = new StoredProfile(getUUID(player), username, true, FloodgateState.TRUE,
-                    getAddress(player).toString());
-        }
-
         //start Bukkit/Bungee specific tasks
         startLogin();

Review it whenever you have time for it. No need to hurry. :)

[Smart123s]

Let's not forget to check this on Velocity since #1071 got merged. This PR only changes code in core, so it's possible, that it'll work without any modifications, but who knows? I'll be pretty busy nowdays, I'll report back once I have some time.

[TuxCoding]

Finally I have some free time so I looked over the code. I would like to merge it now and revive some work on the project. I only inverted some ifs, because for me personally reading the non-inverted-case is easier to read than having shorter code.

In the end, I only have trouble to understand the if-cases in FloodgateManagement that would like to have resolved before merging it. Thank you in advance for the help.

[Smart123s]

Finally I have some free time so I looked over the code. I would like to merge it now and revive some work on the project. I only inverted some ifs, because for me personally reading the non-inverted-case is easier to read than having shorter code.

In the end, I only have trouble to understand the if-cases in FloodgateManagement that would like to have resolved before merging it. Thank you in advance for the help.

Those if statements got really complicated, that's for sure. Some months ago I tried looking into it to simplify the code, but I couldn't really do anything about it.

I have force pushed those changes as a fixup of Differentiate Floodgate players during login & rebased the branch on main, so you may have to re-sign your commits.

EDIT: There were some merge conflicts in the pom.xml versions, duh...

It's nice to hear from you after all this time! Thanks for remembering, and looking into my code after so long Hope you are alright!