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

Implements feature to type password in chat to login/regi… #121

Merged
merged 7 commits into from
Nov 27, 2023
Merged

Implements feature to type password in chat to login/regi… #121

merged 7 commits into from
Nov 27, 2023

Conversation

scaik
Copy link
Contributor

@scaik scaik commented Nov 22, 2023

No description provided.

@bivashy bivashy self-requested a review November 22, 2023 16:09
@bivashy bivashy added the status: needs testing Issue or PR should be tested label Nov 22, 2023
@bivashy
Copy link
Owner

bivashy commented Nov 22, 2023

Hello there! Thanks for your first pull request!
Everything seems to be okay. I also like the way you've implemented it

I'll squash and merge (after testing) and attach your profile link in the GitHub and Spigot releases.

But first of all, please check the review.

@bivashy
Copy link
Owner

bivashy commented Nov 22, 2023

Everything seems to be okay, I'll try to test this as soon as possible.

After testing I'll approve that PR.

@bivashy bivashy requested review from bivashy and removed request for bivashy November 23, 2023 19:28
@bivashy
Copy link
Owner

bivashy commented Nov 24, 2023

I've tested, it is great! Thank you for the implementation of this future.

But there are several problems with register implementation:

  • Player could accidentially register password with whiteline characters, for example if player would type into chat: 'password password', plugin will consider that as 'test test' password instead of 'test'
  • Password confirmation requirement such like in /register <pass_repeat> doesn't work on chat too.

Solution:
Simply split by whitespace before calling PasswordChatEvent

Pull Request that fixes it:
https://github.com/scaik123/MC-Auth-with-Link/pull/1

You could deny Pull Request if you have more elegant solution

@bivashy bivashy self-requested a review November 24, 2023 20:41
@bivashy bivashy removed their request for review November 24, 2023 21:01
@scaik
Copy link
Contributor Author

scaik commented Nov 25, 2023

yeah, that's seems to be a good solution, but i found a probable bug. idk was it intended to be like that, so please, check out the review

@scaik
Copy link
Contributor Author

scaik commented Nov 25, 2023

or we could require player to chat the password two times(type password, press enter, type conf password, press enter) and throw an error if the password contains whitespace

@bivashy
Copy link
Owner

bivashy commented Nov 25, 2023

yeah, that's seems to be a good solution, but i found a probable bug. idk was it intended to be like that, so please, check out the review

Sorry, but i can't see your review if you meant that

or we could require player to chat the password two times(type password, press enter, type conf password, press enter) and throw an error if the password contains whitespace

Nice solution, but most importantly it should be intuitive for user to use.

@scaik
Copy link
Contributor Author

scaik commented Nov 25, 2023

image

bivashy and others added 2 commits November 25, 2023 19:05
[BugFix] Properly handle whitespaces in AuthenticationChatPasswordListener
@bivashy bivashy merged commit d565fae into bivashy:main Nov 27, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs testing Issue or PR should be tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants