-
-
Notifications
You must be signed in to change notification settings - Fork 109
Account Login and Verification Support #835
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
base: develop
Are you sure you want to change the base?
Conversation
Introduces new UI windows and backend logic for CnCNet account login, nickname management, and user verification via the CnCNet API. Player list now displays verified status, and configuration options for API usage and endpoint are added.
Introduces a 'verified accounts only' setting for CnCNet multiplayer game creation and lobbies. Updates UI to allow hosts to restrict rooms to verified CnCNet accounts, enforces restriction on join and in the lobby, and propagates the setting through game creation and broadcast logic.
string[] splitMessage = msg.Split(new char[] { ';' }); | ||
|
||
if (splitMessage.Length != 12) | ||
if (splitMessage.Length < 12) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh? Why changing the !=
operator instead of adding the number here?
Nightly build for this pull request:
|
{ | ||
Name = "lblWindowTitle", | ||
FontIndex = 1, | ||
Text = "CONNECT TO CNCNET" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L10N all the visible-to-user strings for translations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pr'd this as draft with some things still needing to be polished. Can work on translations once everything seems good to go
Implement windows auth tokens alongside linux auth tokens for checkmark verification in nick lists
// Trigger verification after user list is received, if API is enabled | ||
if (ClientConfiguration.Instance.UseCnCNetAPI) | ||
{ | ||
currentChatChannel.UserListReceived += VerifyUserList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UserListReceived is a list of users on the IRC channel with no ident info which would make verification impossible here if I'm not mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so yeah. I used grants pr from 6 years ago for this and tried to update it. Having to go back and update things.
{ | ||
var request = new NameValueCollection(); | ||
request.Add("email", email); | ||
request.Add("password", password); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hash password with salt pls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for an HTTPS connection. Only the database needs to hash the password with salt and it is out of the client's concern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for an HTTPS connection. Only the database needs to hash the password with salt and it is out of the client's concern
No real password should be transferred through the Internet, so client should implement hashing with salt too IMO.
HTTPS needs to remove MitM attack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dude, HTTPS is considered safe, without mitm attacks unless the system ca is compromised
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dude, HTTPS is considered safe, without mitm attacks unless the system ca is compromised
In Russia we have installed certificates from our local CA center that allows to get access to the banking sites and potentially get us under MittM HTTPS attack. So that is why I suggest to use HTTPS and hashing with salt to protect user's data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all right...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you hash on the client side, the hashed password becomes the actual password (with the hashing algorithm being nothing more than a means to convert a user-held mnemonic to the actual password).
This means that you will be storing the full "plain-text" password (the hash) in the database, and you will have lost all benefit of hashing in the first place.
If you decide to go this route, you might as well forgo any hashing and simply transmit and store the user's raw password (which, incidentally, I wouldn't particularly recommend).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really. We can have two layers of hashes. The database stores the 1st hash in plaintext; the client generates the 2nd hash from the 1st hash. 1st hash can be calculated using raw password and the salt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not what mah_boi proposes though.
the client generates the 2nd hash from the 1st hash.
doesn't that require transferring the hashed password/salt from server to client? isn't that a big no-no as the client could be malicious and use the obtained correct password hash to calculate locally what the password is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. the 1st hash is calculated from the raw password -- it won't be transmitted from the server to the client. The server stores the 1st only to enable an HTTP-digest-like challenge, removing the need of transferring plaintext password as well as removing the need to store the plaintext password in the database
Btw there should be button |
@CnCRAZER @SadPencil @SadPencil @Metadorius I find that FF's client ladder as lobbies with ladder icon in the regular lobby list (that neogrant's implement in old days) is a bit hacky instead of what we have right now in YR package -- separated button for quick matches in main menu. I suggest the future client's implementation of ladder API should follow the same idea -- separate window for searching quick matches in the ladder. |
The register button takes you to the ladder account creation page. Not the exact same thing but meh |
While I don't have much experience with FF's client ladder implementation, I disagree with the notion that the concept itself is bad.
|
I wasn't expecting so much feedback so quickly lol. So, currently: user has to go into an active ladder table and be sure they have an active username for the ladder to have the username show up in the login window. Am in talks with ladder api team to make a new non ladder api for generic lobbies. Will wait for that before I do any more changes I think |
Also curious on y'all's thoughts. @Metadorius @SadPencil @MahBoiDeveloper Lets say a user decided to not use a cncnet account and use a nick of "xyz", but then a verified account logins with the same nick. Should the unverified account be kicked or told to change their name? Is this even possible? lol |
I believe that provide users logging only with email+password would solve issue. Currently if unregistered user would connect with the same name that already exist in the IRC chat, it would be renamed with If we talk about guests nicknames vs authorized nicknames, I prefer to kick guests or add them postfix like |
I think the client could make requests to the API when logging in online, and if such account name exists as verified account -- then add an underscore like it does now. |
Establishes 2 new flags within
ClientDefinitions.ini
:UseCnCNetAPI
CnCNetAPIUrl
Setting
UseCnCNetAPI
toYes
orTrue
allows online login window to connect to cncnet ladder account and retrieve active nicknames. Setting this flag toNo
orFalse
hides this field entirely.Verified Account Only Option