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

Premium(license, online) support added #125

Closed
wants to merge 14 commits into from
Closed

Premium(license, online) support added #125

wants to merge 14 commits into from

Conversation

scaik
Copy link
Contributor

@scaik scaik commented Nov 27, 2023

Pull Request Etiquette

  • I have checked the PRs for upcoming features/bug fixes.

Changes

  • Internal code
  • API (affecting end-user code)
  • Other: New configuration fields added

Closes Issue: #39

Description

  • Added new command: /license <on/off>: Enables/disables online mode enforcing for the player himself. Adds player to PendingPremiumAccountBucket for specified in config time. Player is gonna have to reconnect to the server from premium account. If time runs out, player is gonna be able to connect from cracked account again.
  • On player handshake(PreLoginEvent), if player has premium(license) enabled or player consists in PendingPremiumAccountBucket, forces online mode for the connection. If player is in the bucket, on post login account is gonna be set to premium, now enforced online mode is enabled for the player.
  • New config field: enable-license-support and license-verify-time. First one enables /license command and online mode verification for premium players. Second field sets time for pending premium player to verify license account. Basically when player types /license passwd on, it kicks player and forces online mode for him. After license-verify-time runs out, player is gonna be able to relogin from cracked account again, premium wont be enabled for him.
  • Small other changes like changepass command argument name

@scaik
Copy link
Contributor Author

scaik commented Nov 27, 2023

Omg i just realized that it included commits from previous PR idk why, how do i get rid of them?

scaik and others added 2 commits November 28, 2023 02:24
…ommand and premium account process(PreLoginEvent, ServerConnectedEvent)
[BugFix] When license support is disabled in config, block /license c…
@bivashy
Copy link
Owner

bivashy commented Nov 27, 2023

Omg i just realized that it included commits from previous PR idk why, how do i get rid of them?

You could create branch with "premium account" features and create new pull request.

But i dont think that (redundant commits) would have big impact

@scaik
Copy link
Contributor Author

scaik commented Nov 27, 2023

yeah, i accidentally pushed to main branch. next time i will create new branch)

@scaik
Copy link
Contributor Author

scaik commented Nov 28, 2023

@bivashy so what, is this alright, or i should add something else?

@bivashy
Copy link
Owner

bivashy commented Nov 28, 2023

@bivashy so what, is this alright, or i should add something else?

Hi, currently i don’t have time to review this Pull Request. But i haven't seen handle of the case when player changes his username.

You can look at LibreLogin, which handles every conflict, and name change

@scaik
Copy link
Contributor Author

scaik commented Nov 28, 2023

well, to handle name change properly you have to switch from username to uuid in the config anyway. i can check if player name have changed on prelogin and change account username to his new name.
anyway, gonna wait for your future review and tests

@bivashy bivashy self-requested a review December 2, 2023 11:04
@@ -26,7 +27,7 @@ public class ChangePasswordCommand {

@DefaultFor({"passchange", "changepass", "changepassword"})
@CommandCooldown(CommandCooldown.DEFAULT_VALUE)
public void changePlayerPassword(ServerPlayer sender, DoublePassword password) {
public void changePlayerPassword(ServerPlayer sender, @Named("пароль") DoublePassword password) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could somehow make localized named annotation using Annotation replacers?

@DefaultFor({"license", "premium", "authlicense"})
@AutoComplete("* @onOff")
@CommandCooldown(CommandCooldown.DEFAULT_VALUE)
public void changePlayerPassword(ServerPlayer sender, @Named("пароль") String password, @Named("Вкл/Выкл") String onOff) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could somehow make localized named annotation using Annotation replacers?

@bivashy
Copy link
Owner

bivashy commented Dec 2, 2023

First of all, great job! You've tackled the feature I was hesitant to approach.

It seems you've implemented a mechanism that forces players into online mode if they have a pending premium account. Overall, the implementation is impressive.

However, there are a few aspects I'd like to modify or avoid. To address these, I plan to submit a Pull Request to your repository with the necessary changes. Please note, my review is intended as constructive feedback, not personal criticism. It's meant to make it easier for others to understand and work with the code.

Future Plans:
I'll keep this pull request open as there are specific issues I want to resolve.

Task List for Improvement:

  • Support Name Changes: Ensure name changes are supported, even if id-type in the configuration is set to NAME. This might require modifications in the AccountDatabase implementation.
  • Annotation Replacement: Substitute all @Named annotations with a custom annotation that can support localized messages.
  • Refactor Certain Getters: Refrain from using boolean isPremium in some getters; this may necessitate a refactor.

Good luck!

@scaik
Copy link
Contributor Author

scaik commented Dec 7, 2023

glad you are interested! im sorry, i didn't have time to do this before. i will look into it in 1-2 days i think

@bivashy
Copy link
Owner

bivashy commented Dec 19, 2023

Hello, maybe we should convert this Pull Request to Draft?
Because it is still WIP (Work in progress).

I can do it myself, you can just place thumbs up emoji on my comment, and I'll convert this Pull Request into Draft.

@scaik
Copy link
Contributor Author

scaik commented Dec 25, 2023

yeah, im sorry, i dont have much time right now. if this is gonna be still relevant, i can do this feature after the holidays

@bivashy
Copy link
Owner

bivashy commented Dec 25, 2023

No problem, there is no hurry, I'll try to contribute on this too when I'll have time.

Happy christmas!

@scaik
Copy link
Contributor Author

scaik commented Dec 25, 2023

you too!

@scaik scaik marked this pull request as draft March 8, 2024 17:38
@scaik scaik marked this pull request as ready for review March 8, 2024 19:53
@scaik
Copy link
Contributor Author

scaik commented Mar 8, 2024

hi, im back for now.
resolved your remarks about getters and jodatime, but i couldn't manage to replace @nAmed with annotation replacer, there are not enough docs for me about them to implement this feature.
and about player changing his username, i wouldn't allow him to login because that is not safe. most of the plugins handle players by their username, not uuid, so a player can imitate/replace his uuid and gain admin access.

@bivashy
Copy link
Owner

bivashy commented Mar 8, 2024

hi, im back for now. resolved your remarks about getters and jodatime, but i couldn't manage to replace @nAmed with annotation replacer, there are not enough docs for me about them to implement this feature. and about player changing his username, i wouldn't allow him to login because that is not safe. most of the plugins handle players by their username, not uuid, so a player can imitate/replace his uuid and gain admin access.

Hello there, annotation replacer documentation: https://github.com/Revxrsal/Lamp/wiki/Annotation-replacers

Also you are not right about nicknames and UUID, players cannot change their UUID, because:
If server runs as offline mode, player UUID is UUID.fromString("OfflinePlayer: " + name) in UTF-8:
https://github.com/PaperMC/Velocity/blob/8891faa52c85c165ce37540adc236f4678617b69/api/src/main/java/com/velocitypowered/api/util/UuidUtils.java#L52-L54

If server runs as online mode, player UUID is generated by Mojang: https://wiki.vg/Mojang_API#Username_to_UUID

@scaik
Copy link
Contributor Author

scaik commented Mar 9, 2024

oh, right.
but anyway, if player with license on changes his name to, for instance, admin's nickname. he will probably gain admin permissions

@bivashy
Copy link
Owner

bivashy commented Mar 11, 2024

oh, right. but anyway, if player with license on changes his name to, for instance, admin's nickname. he will probably gain admin permissions

Yes, but we can resolve that too, we could create "strategy" for profile conflict (when offline and online accounts has same name), you can see it here: https://github.com/kyngs/LibreLogin/wiki/Profile-Conflicts

And probably use code from this repository too

@scaik
Copy link
Contributor Author

scaik commented Mar 12, 2024

Alright, im going to be working on it. But i couldn't manage to make annotation replacers work, im sorry.

@bivashy
Copy link
Owner

bivashy commented Mar 12, 2024

Alright, im going to be working on it. But i couldn't manage to make annotation replacers work, im sorry.

No problem, its ok

@bivashy bivashy marked this pull request as draft April 3, 2024 16:41
@scaik scaik closed this by deleting the head repository Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants