Call for participation: Lobby account password change functionality

[Dunedan]

Up to now 0ad doesn't offer the ability for users to change their passwords for the multiplayer lobby. However, that'd be a great feature to have and has in fact been already a feature request for several years (https://trac.wildfiregames.com/ticket/2543).

I had a quick look and believe this should be pretty straight forward to implement, as the server-side as well as the XMPP library used by 0ad already support that. So all left to implement it is to add some glue code to 0ad and build the UI for it.

If you're interested in helping out and implementing this feature or have any questions, please reply to this post or send me a PM.

[rossenburg]

@Dunedan i tried to do what I can about this feature but i have a question, to avoid someone else resetting other person's account password. Is there a need to make some sort of extra verification to confirm the account really belongs to that specific user before allowing a user to go ahead and reset the password?. For example, "i forgot my password, i made a dummy ui which shows "Forgot password?" at the login screen, when a user clicks that,  passwordReset.xml (a sprite to handle the reset inputs )should open which should then have 3 input fields, username , new password , confirm new password. But what if i enter a username of another person, it will just ahead and allow me to reset so if there a need to confirm the identity of the person before even allowing to reset password? If we had info like email linked to all individual account we could just send something like OTP to verify users and would be easier but in this case that we don't. Even if i decide to use data from user.cfg and disable the username field with a default value from the config file, it can still be modified , what do you suggest?

Edited August 2, 2022 by rossenburg

[smiley]

10 minutes ago, rossenburg said:

Is there a need to make some sort of extra verification to confirm the account really belongs to that specific user before allowing a user to go ahead and reset the password?

We currently don't collect enough information to implement a "forgot password" feature. This is for changing passwords after the account has already been authed. Which could be implemented via another tab to the prelobby page or something inside the lobby interface, I am not sure.

[Darkcity]

To make a better solution, we need some personal info from user like email id (although optional) while creating the account. User can use same email to reset password. But as discussed in many threads, @Stan` mentioned that we need to change T&C and complie with GDPR rules. Some workaround is required there. Not sure how much effort but @Stan` can comment on that.

[rossenburg]

1 hour ago, smiley said:

We currently don't collect enough information to implement a "forgot password" feature. This is for changing passwords after the account has already been authed. Which could be implemented via another tab to the prelobby page or something inside the lobby interface, I am not sure.

i bet that should work, if this process is not about forgotten password but to allow an active user session to be able to reset their password ( whiles logged in ), then i guess it isn't a big deal. Thanks for clarifying but i guess it will be much better to focus on users who are logged out of their account since those who can already access their accounts are less likely to reset their passwords @smiley. And aside that, allowing users to change account passwords without any further verification (either there's active session or not) will just promote more stress on the server since i can change my password 10times in 2mins, unless maybe we think of adding something like throttle middleware or password reset cooldown to the whole process

Edited August 2, 2022 by rossenburg

[Dunedan]

Sorry for being not clear about that, but as @smileyalready clarified I was talking about changing the password after successful authentication. So this thread is not about a "forgot password" feature or how to also collect a users email address during registration, as that would be much more effort, but solely for allowing an authenticated user to change his password. That should be, as mentioned in my initial post, pretty straight forward. Please stay on that topic here and discuss additional ideas in other threads.

[Dunedan]

2 hours ago, rossenburg said:

And aside that, allowing users to change account passwords without any further verification (either there's active session or not) will just promote more stress on the server since i can change my password 10times in 2mins, unless maybe we think of adding something like throttle middleware or password reset cooldown to the whole process

Changing your password 10 times in 2 minutes does not stress the server running the lobby at all. Much more often might, but we have rating limiting against such DoS attacks in place, therefore considering any additional rate-limiting isn't necessary.

[Darkcity]

Thanks for claifying. When this feature being deployed, warn players about not sharing their account details with other players becasuse after this feature, they can change the password and will never share credential with original player :p. 

Edited August 2, 2022 by Darkcity

[Dunedan]

27 minutes ago, Darkcity said:

Thanks for claifying. When this feature being deployed, warn players about not sharing their account details with other players becasuse after this feature, they can change the password and will never share credential with original player :p. 

I believe that won't be necessary, as sharing accounts isn't permitted anyway and loosing access to a shared account does already happen right now when the account gets banned for any reason.

[G.O.A.T]

As far as I know some leaked account publicly were not banned for long time. If you need someone to act fast/er, shall I have a ban ability? (just joking)

Changing password is great step forward.

I want to publicly thanks to developers, we hate them more often, but they also deserve respect!

 

[Dunedan]

*bump*

Anybody interested in contribution password change functionality? If there are any roadblocks regarding that just let me know.

[Stan`]

42 minutes ago, Dunedan said:

*bump*

Anybody interested in contribution password change functionality? If there are any roadblocks regarding that just let me know.

I'm not sure what is missing in the engine would be nice to figure that out

[smiley]

I have a patch made last night, but the problem with how our XmppClient is setup means that it becomes very ugly.

We can summarize three modes for it. Currently using two.

* Registration.

* Login and join MUC.

* Login and don't join MUC. 

Join MUC implies joining the room and switching to the chat interface.

Currently only the first two modes are used and is handled by passing in a boolean flag in the XmppClient constructor.

This is already ugly as having to create the client object in two modes lead to cases where we cannot reuse the client to connect to the MUC upon registration and have to tear it down before remaking it.

Adding the third state is extremely ugly as it involves making numerous other checks to cherry pick the needed behavior. If we go with the flag approach, it also means the mode can no longer be boolean but that's the least of our concerns. This is what I did last night by using an enum flag based thing instead. JOIN_MUC | REGISTERING. But it was just way too much of a mess.

I think what we need to do is make XmppClient an actual singleton which isn't constructed based on the behavior we need. And than we can instead call xmppRegister, xmppLogin, xmppChangePassword etc rather than constructing three slight variations of it for each mode. This would still allow maintaining the current GUI to XmppClient interface untouched. I would try this next.

The actual password change functionality via gloox is max 20 lines but it needs some refactor on how the XmppClient currently works.

[smiley]

Disclaimer: This would be a massive change of course, and not related to the topic, but perhaps interesting so posting. This is not needed to implement password change.

Maybe, the whole client needs to be slightly cleaned up by encapsulating related things rather than having JS separately query every detail. This would result in an interface where the single source of truth about lobby entities now lie within the client and entities/gamedetails could directly be exposed to JS. This should drastically minimize the number of events too as nick changes, presence changes and game status changes could be automagically updated along with the exposed JS representation (Need to check if this could actually be a thing, but it should be. We do pretty much the same thing for param nodes.)

The new interface could look somewhat like this I guess.

class XMPPUser {
private:
    std::string m_jid;
    std::string m_username;
    u16 m_rating;
    u16 m_gamesPlayed;
    u16 m_gamesWon;
    u16 m_gamesLost;
};

class XMPPGame {
private:
    std::string m_hostJID;
    std::map<std::string, std::string> m_gameData;
};

class IXMPPClient2
{
public:
    virtual IXMPPClient2::~IXMPPClient2() = default;

    // Allow unauthenticated
    virtual bool Login(const std::string& username, const std::string& password) = 0;
    virtual bool ChangePassword(const std::string& newPassword) = 0;
    
    // Requires being authenticated
    virtual void Logout() = 0;
    virtual bool Register(const std::string& username, const std::string& password) = 0;

    // Restrict clients to join only one MUC at a time.
    virtual bool JoinMUC(const std::string& room) = 0;
    virtual bool LeaveMUC() = 0;

    // Basic XMPP functions
    virtual bool SendMessage(const std::string& toUsername, const std::string& message) = 0;
    virtual bool SendMessageToMUC(const std::string& message) = 0;
    virtual bool SetNick(const std::string& nick) = 0;
    virtual bool SetPresence(const std::string& status) = 0;

    // Member info
    virtual const XMPPUser& GetUser(const std::string& username) const = 0;
    virtual const std::list<XMPPUser>& GetUsers() const = 0;

    // Pyrogenesis functions
    virtual const std::list<XMPPUser>& GetLeaderboard() const = 0;
    virtual bool RegisterGame(const XMPPGame&) = 0;
    virtual bool UnRegisterGame(const XMPPGame&) = 0;
    virtual bool ReportGame(const XMPPGame&) = 0;
    virtual const XMPPGame& GetGame(const std::string& hostUsername) const = 0;
    virtual const std::list<XMPPGame>& GetGames() const = 0;
};

[Dunedan]

10 hours ago, smiley said:

I think what we need to do is make XmppClient an actual singleton which isn't constructed based on the behavior we need. And than we can instead call xmppRegister, xmppLogin, xmppChangePassword etc rather than constructing three slight variations of it for each mode.

While I'm not that familiar with the Pyrogenesis side of things, that sounds like it'd make a lot of sense.

6 hours ago, smiley said:

// Allow unauthenticated
    virtual bool Login(const std::string& username, const std::string& password) = 0;
    virtual bool ChangePassword(const std::string& newPassword) = 0;

AFAIK changing the password is only possible after authenticating.

6 hours ago, smiley said:

    // Pyrogenesis functions
    virtual const std::list<XMPPUser>& GetLeaderboard() const = 0;
    virtual bool RegisterGame(const XMPPGame&) = 0;
    virtual bool UnRegisterGame(const XMPPGame&) = 0;
    virtual bool ReportGame(const XMPPGame&) = 0;
    virtual const XMPPGame& GetGame(const std::string& hostUsername) const = 0;
    virtual const std::list<XMPPGame>& GetGames() const = 0;

I guess you're aware, but there are several Pyrogenesis related functions (like "ChangeGame" and "GetProfile") missing.

As a disclaimer: I'm currently working on the server-side for PubSub (https://trac.wildfiregames.com/ticket/4203), which will require some additional changes for Pyrogenesis (like subscribing for the relevant PubSub nodes), however I believe that should happen separately and after such a refactoring.

[smiley]

7 hours ago, Dunedan said:

but there are several Pyrogenesis related functions (like "ChangeGame" and "GetProfile") missing.

I am envisioning exposing the XMPPGame object directly. The logic for sending the actual stanzas would then be a method on XMPPGame. For get profile, I absorbed the information into XMPPUser. GetUser would first lookup the xmpp user and then query the elo related data and construct the User object with all relevant information. GetProfile would no longer be needed as a separate method.

7 hours ago, Dunedan said:

AFAIK changing the password is only possible after authenticating.

Indeed. I missed it at first then hastily added it under the wrong comment before posting. Register() and ChangePassword() were swapped.

Separating out these from the current monolothic client would allow to update the underlying mechanisms more easily. XMPPGame related methods only are subjected change if we switched from XPartaMuPP to PubSub events while the XMPPClient requires no changes.

https://github.com/livvv2k/0ad/tree/xmpp_client, any work on this front would go here.

[Dunedan]

4 hours ago, smiley said:

I am envisioning exposing the XMPPGame object directly. The logic for sending the actual stanzas would then be a method on XMPPGame. For get profile, I absorbed the information into XMPPUser.

All right, gotcha.

[Dunedan]

*bump* Any progress here or anybody else who want to take up this topic?

[rossenburg]

On 06/09/2022 at 8:23 PM, smiley said:

I am envisioning exposing the XMPPGame object directly. The logic for sending the actual stanzas would then be a method on XMPPGame. For get profile, I absorbed the information into XMPPUser. GetUser would first lookup the xmpp user and then query the elo related data and construct the User object with all relevant information. GetProfile would no longer be needed as a separate method.

Indeed. I missed it at first then hastily added it under the wrong comment before posting. Register() and ChangePassword() were swapped.

Separating out these from the current monolothic client would allow to update the underlying mechanisms more easily. XMPPGame related methods only are subjected change if we switched from XPartaMuPP to PubSub events while the XMPPClient requires no changes.

https://github.com/livvv2k/0ad/tree/xmpp_client, any work on this front would go here.

When a user is authenticated, do we need to reinitialize the XmppClient? I'm asking this because I made few attempts trying to get this feature running.

The idea is to allow authenticated users to be able to reset their password, probably the "forgotten password" feature will follow up later on (when users do not remember their password and are logged out of their accounts). For extra security, users are asked to confirm their old password before updating the old password ( just so, if someone else has an account that doesn't belong to them they won't be able to change password ). I dunno if its best to compare the encrypted form of the old password in other to achieve this since its stored locally or there's more efficient way.

Also check if users are using their old password as new password - if new password is the same as old password it shouldn't allow ( since in that case nothing has changed )

 

and thirdly, the question do we need to reinitialize XmppClient before we can use  XmppClient function - I thought an active session already has XmppClient initialized since

Engine.HasXmppClient() returns true when user is authenticated. What's the right way to tackle this ? One more thing i noticed and wishes its prevented is allowing users to log into accounts with the encrypted form of their password instead of the plain passwords, i feel like it might help alot @smiley @Dunedan

[Dunedan]

On 19/10/2022 at 7:26 PM, rossenburg said:

For extra security, users are asked to confirm their old password before updating the old password ( just so, if someone else has an account that doesn't belong to them they won't be able to change password ). I dunno if its best to compare the encrypted form of the old password in other to achieve this since its stored locally or there's more efficient way.

AFAIK neither gloox nor ejabberd support checking the current password during password changes, because you can change your password anyway only when already being authenticated. So to implement that you either have to check against a locally stored password or do some re-authentication during the password change.

Checking against a local stored password is something which can be circumvented with a patched version of 0ad and isn't straight forward anyway, because storing the XMPP password is optional. Doing re-authentication with the XMPP-server during password change adds a lot of additional complexity and isn't as it's usually done with XMPP.

Also keep in mind that most players which have stored their password in 0ad probably don't even know anymore what their password is and therefore wouldn't be able to change their password at all, if a password change would require typing in the current password.

What would be the benefit of checking the current password anyway? In which situations would that provide a benefit? The only situation I can imagine right now is when granting an untrusted person access to your computer, but that's a case we shouldn't try to handle. If an untrusted person has access to your computer they can also simply install a keylogger and get the password that way.

My suggestion is to keep the initial implementation as simple and straight-forward as possible. Avoid stuff which adds complexity in terms of code as well as in terms of user experience if it doesn't provide a significant benefit.

 

[rossenburg]

11 hours ago, Dunedan said:

Also keep in mind that most players which have stored their password in 0ad probably don't even know anymore what their password is and therefore wouldn't be able to change their password at all, if a password change would require typing in the current password.

Understood, but just to be clear. we are dealing with 2 things here right?

1. Users who already have access to their accounts and wishes to update their password ( just incase someone had their password by any means) - this takes place in lobby when user is logged in
2. Users who do not remember their passwords and are stuck in the log in page 

or just to make things even more simpler, just one page in settings ( where both cases ( 1 and 2 in ) , can go ahead and update their passwords ). I believe there's a need to somehow verify that someone who has case 1 , really owns the account they wish to update the password of. Else i can just go ahead and reset someone else's password or something..I dunno if im thinking way too broad, can you suggest how the whole process should be? , if possible in some kind of pseudocode or step by step listing 

[Dunedan]

21 hours ago, rossenburg said:

Users who do not remember their passwords and are stuck in the log in page 

The feature to be able to change the password, won't help such users at all, as they can't proof that the account they want to change the password for is their own account.

There is a difference though between users who don't remember their password and don't have it stored in 0ad (these are the ones which can't change their password) and users who don't remember their password, but have it stored in 0ad (those ones can still log in and should also be able to change it).

21 hours ago, rossenburg said:

I believe there's a need to somehow verify that someone who has case 1 , really owns the account they wish to update the password of. Else i can just go ahead and reset someone else's password or something

How would you do that? As you don't have their password you can't be logged in into their account, which is a prerequisite to changing the password.

21 hours ago, rossenburg said:

I dunno if im thinking way too broad, can you suggest how the whole process should be? , if possible in some kind of pseudocode or step by step listing 

Essentially it's the same as for most websites which offer accounts: To change your password, you have to log in first. Once logged in there is somewhere a form you can use to change the password. Note that this is different from a password reset process which usually doesn't require a login, but something like a recovery email instead. That's a use case we can't cover for now.

[smiley]

Since the refactor mentioned above would take a while, and this feature seems simple enough and quite necessary, I have done the XMPP client changes, https://code.wildfiregames.com/P288. It needs an interface that can also handle a nicer way of prompting users to reconnect.

Engine.LobbyChangePassword(newPassword); // Pass in the hashed string as done in registration code and update config

[Stan`]

cc @Dunedan

[Stan`]

@Dunedan you asked for someone to write code to add that feature. The diff above does the C++ part it would seem ?