Floodgate support for Bukkit - #494
Conversation
The Floodgate API requires UUID which is inaccessible at the level FastLogion operates on. A workaround for this is to check if the currently connecting player is also a part of the Geyser server's online players list. *TODO: Check for Java and Bedrock name conflicts with multiple configurations.*
This is buggy in most cases.
Check config.yml for details.
Knwon Bug: Profile.isSaved() is 'false' when logging in from Bedrock after auto registering through Bedrock so FastLogin will try to register for a second time, insted of logging in
|
There is an issue unrelated to this fork which makes the building of the Bungee part of the plugin fail. The fork can still be tested as it does not touch the Bungee version. For more information, see this build report from the upstream version of the plugin. |
TuxCoding
left a comment
TuxCoding
left a comment
There was a problem hiding this comment.
For more information, see this build report from the upstream version of the plugin.
I noticed that. I hoped the author would fix it. I'll go to look into an alternative.
Auto Registering from a Bedrock clinet will cause a Java Client with the same name to also be registered, but will not be automatically logged in
The ForceLoginTask will register and login the player at the same time. So I wonder what went wrong here or are they separate login events?
Profile.isSaved() is 'false' when logging in from Bedrock after auto registering through Bedrock so FastLogin will try to register for a second time, insted of logging in
Try the auth plugin isRegstered method
Maybe put them into a common function to avoid code duplication?
You could also put the stuff into an extra class in order to isolate it. This could make it easier to test against it.
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
|
|
There was a problem hiding this comment.
Could you move the dependencies up, because they are not auth plugins.
There was a problem hiding this comment.
I've moved the dependencies above <!--Login Plugins--> and left the repositories alone.
Please check if they in the right place now.
| for (GeyserSession geyserPlayer : GeyserConnector.getInstance().getPlayers()) { | ||
| if (geyserPlayer.getName().equals(player.getName())) { | ||
| // this also returns a floodgatePlayer for linked Java accounts | ||
| // that's why the Geyser Server's player list also has to be checked |
There was a problem hiding this comment.
Does this mean you have tried the getPlayerByUUID method? This would save us this loop check.
There was a problem hiding this comment.
According to my tests, FloodgateAPI.getPlayer(player.getUniqueId()) also returns a FloodgatePlayer instance if the player is connecting from Java Edition (JE) but has a linked Bedrock Edition (BE) account.
Later in the code floodgatePlayer != null is used to decide if a player is connecting from BE. But since JE accounts with linked BE accounts also have a return value, I had to filter them out. That's what the for loop is used for.
I couldn't find a better solution (yet), but honestly, I didn't spend much time looking. I am planning to figure out an alternative method for this. If I won't find anything better, than at least I'll try switching name to uuid in line 60.
| player.getName()); | ||
| String allowNameConflict = plugin.getCore().getConfig().getString("allowFloodgateNameConflict"); | ||
| // check if the Bedrock player is linked to a Java account | ||
| boolean isLinked = floodgatePlayer.fetchLinkedPlayer() != null; |
There was a problem hiding this comment.
This appears to be a blocking request, so it has to be async in order to not block the main thread.
There was a problem hiding this comment.
I've moved this to an asynchronous task.
Co-authored-by: games647 <games647@users.noreply.github.com>
I have opened another Pull Request that could be an alternative to that. |
Can I create a |
That's normally for auth plugins. Maybe we should choose a better name for that. You could put into the auth package, because it's related to login/auth handling. |
previously if autoRegisterFloodgate was set to false and the player was not registered, the plugin tried to auto-login the user, which has led to misleading "Failed to login" messages
|
I need your opinion on this one (and on everything else of course):
Is it good this way, or should only one of them be able to auto log in? |
|
Okay, so I think that the PR is ready (well, besides a new bug that I've added to the Known bugs list, any idea on how to fix it?) for another review, and for merging if you find everything alright. Could you please check the tasks that I've left unticked? Also, I've commented on all of your "change requests", could you close them if you are satisfied with my solutions? Please, don't forget to properly test my code before merging it, I'm not sure if I hadn't broken anything, and I don't want to compromise anyone's server's security. Also, are the configuration options understandable? |
|
Hi, i tried compiling your fork but it still removes the floodgate prefix, any fix for this as of yet? |
No, nothing yet. I'd really like to tell you to view to #493 but there's no usable information there either. |
|
Thanks for the fast response. I guess i will just stick to bungeecord for now. |
The FloodgateApi is the same for Bukkit and Bungee, so the Floodgate related code could be used in a future Bungee implementation too. Currently, Bungee will report Floodgate disabled to core, so the upstream Floodgate implementation will be used there. If enough code will be moved to core, I might consider enabling these features to BungeeCord too.
IntroductionOkay, so at this point I want to do a little bit of heads-up about this Pull Request, where it is, where it's going, what are the plans for the future, and what I have done lately. Where did it start?My initial plan with this PR was to implement Floodgate support for Bukkit without even touching BungeeCord and/or Core, so I wouldn't break Bungee. It's worth noting that I haven't even tried the Bungee version of FastLogin back then. Well, a lot of things changed since then. Supporting BungeeCordYesterday, I took a look at #498 and realized that the features I've been working on (namely autoLogin, autoRegister and floodgateNameConflict) are not present in Bungee, but could be implemented in a similar way to it's Bukkit counterpart. In fact, the FloodgateApi is the same for all platforms. Moving code to coreSo it got me thinking that most of my code could be reused for a BungeeCord implementation of the above mentioned features. Today, I decided to move the Adding Bungee features in this Pull RequestAs I've mentioned earlier, this PR was supposed to be Bukkit only (look at the title of the PR), however I think that at this point it would be reasonable to expand it, and add configuration options to BungeeCord too. Question 1: Should I do it in this PR, or should I keep this Bukkit only, and open another PR for BungeeCord? Things that are not going to be doneI think that a proper fix for #493 is out of scope for this PR. I've looked at it several times and it seems to be an upstream issue somewhere. A workaround could be to make FastLogin assign the prefixes instead of Floodgate. I haven't tried it, so I don't know how complicated would it be to achieve that. However, a drawback of this approach could be that if the original bug goes away, then we'll have to deal with duplicate prefixes. Question 2: Should I implement the workaround for Floodgate To DoThere are two tasks in the original PR message that haven't been completed yet:
There are two reasons why this is important. Firstly, Floodgate name prefixes are not obligatory and can be turned off. Secondly, #493 is still present.
That's actually a hard one, because if I sign out of my Microsoft account on my phone, the it won't let me connect to any server. My idea is to find a cracked version of the game which supports joining cracked servers too. Or if anyone has another idea on how to test this, please let me know. Or if someone could post actual test results, it'd be really awesome. Closing thoughts@games647 could you please answer the two questions that I've asked above. I've also left comments on the changes you requested. Could you please check if those are OK now? Thank you for reading this rather long comment. Have a nice day. |
Keep it for Bukkit. You shouldn't blow up a pull request. PRs in reasonable atomic steps are much better. They are easier to manage and merge. You can always expand later. Think of it like a little bit bigger than commits. New features, bug fixes that already provide an improvement should directly be integrated into it. It makes it easier to test.
Create a new PR for that.
It could be related to faking the receiving of a
Looks good to me. |
Ok, you are right. I'll add a warning to
It looks like I forgot to mention (although I though that I've done it before, sorry about that) that I've opened a ticket GeyserMC/Floodgate#143. In short, the prefixes break after registering an async |
|
I've reverted the commit where I have moved some code to Core since that part turned out to be useless for BungeeCord, so I thought that it shouldn't be merged. |
I experienced that somewhere else too, but I don't remember it. There is the importance of naming commits clearly. Could also be a ProtocolLib issue in this case, depending how the listener actually differs from the synchronous solution. |
Rebased to remove Bungee related changes
This reverts commit b0ef1a5.
Floodgate 1.0 does not work with the current Bukkit implementation. ProtocolLib and Floodgate don' play along nicely when used with FastLogin Related issue: TuxCoding#493
|
5 participants
Summary of your change
Adds support for automatically registering and logging in with both linked and standalone Bedrock Players. Auto login is verified by Floodgate and the player's Xbox Live account.
New Configuration options
These options only work on Bukkit/Spigot/Paper servers, not on BungeeCord
autoLoginFloodgate
This enables auto login for every player connecting through Floodgate.
Possible values:
false,true,linkedLinked means that only Bedrock accounts linked to a Java account will be logged in automatically
This value is treated as a string
and requires apostrophesallowFloodgateNameConflict
This enables Floodgate players to join the server, even if autoRegister is true and there's an existing Java Premium
account with the same name
Possible values:
autoRegisterautoRegistermust betruefor this to workautoRegister's name conflict checkingNote: Linking a new account requires players to log in with a non-linked Bedrock account firstEnabling this will make linking new Bedrock players impossible
More information on linking accounts: https://github.com/GeyserMC/Geyser/wiki/Floodgate#account-linking
Releated Floodgate issue: Add ability to directly link to Mojang account on login GeyserMC/Floodgate#37
Floodgate 2.0 uses a different Linking method (called Global Linking) and this is no longer relevant.
This value is treated as a string
and requires apostrophesautoRegisterFloodgate
This enables auto registering every player connecting through Floodgate.
autoLoginFloodgatemust betruefor this to workRelated issue
Implements / fixes #328
Additional issues in the implementation may be caused by #493
There is an issue UNRELATED to this fork which makes the building of the Bungee part of the plugin fail. The fork can still be tested as it does NOT touch the Bungee version. For more information, see this build report from the upstream version of the plugin.This is now fixed.Known Bugs
Auto Registering from a Bedrock clinet will cause a Java Client with the same name to also be registered, but will not be automatically logged inAuto (and not auto?) registering with a Java account and then logging out, and logging in with a bedrock account with the same name will cause the Bedrock account to be auto logged in without linking. See also at Tests - Same bedrock and java usernames.authPlugin.isRegistered()To be tested
All of the points below should be tried with almost every possible combinations of the configuration entries:
autoRegister,premiumUuid,nameChangeCheck,autoLogin,autoLoginFloodgate,allowFloodgateNameConflict,autoRegisterFloodgateSame bedrock and java usernames
username-prefixmake FastLogin treat same named users as if they had different names? (Floodgate player name prefixes are not working with ProtocolLib #493)With unlinked accounts - Same bedrock and java usernames
With linked accounts (after linking) - Same bedrock and java usernames
Different bedrock and java usernames
Miscellaneous
Does the FloodgateAPI return null (used here) if Geyser configremote.auth-typeis set toofflineand Floodgate is installed on the server and......the player is using cracked Minecraft Bedrock (the official client does not allow you to play multiplayer without logging in to Xbox Live) - IF NOT, THIS CAN ALLOW CRACKED MC-BE USERS TO AUTO-LOGIN WITH ANY USERNAME...the player is logged in to Xbox Live when connecting - This should be fineAdded
GeyserConnector.getInstance().getDefaultAuthType() == AuthType.FLOODGATEwhich should solve this problem.To Do
This Todo list is incomplete, please help me add elements to it.
EDIT: the use cases point to the removed Floodgate 1.0 implementation.
This behavior will be made configurable in a future Pull Request.