Add: GMCP authenticate spec implementation by vadi2 · Pull Request #7152 · Mudlet/Mudlet

vadi2 · 2024-02-17T11:07:16Z

Brief overview of PR changes/additions

Add the GMCP authenticate spec and client credentials: https://wiki.mudlet.org/w/Standards:GMCP_Authentication

Motivation for adding to Mudlet

To solve the issues we have in trying to login to games - not every game accepts "character\npassword" kind of login - while not getting into a security quagmire that comes with exposing passwords to the Lua subsystem.

Other info (issues closed, discussion etc)

add-deployment-links · 2024-02-17T11:07:52Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

… into add-gmcp-authenticate

mpconley · 2024-02-25T03:56:57Z

Please update this so it will produce a fresh Mac build, thanks!

vadi2 · 2024-02-25T08:09:44Z

/refresh links

vadi2 · 2024-02-25T08:10:34Z

@mpconley there you go.

vadi2 · 2024-03-01T06:50:41Z

It's updated, could you force reload the page? Mediawiki caching is a bit weird.

I used Mermaid to put it together - see https://mermaid.live. Always been a plantuml fan, it's Java rendering engine doesn't make it suited for the web - and that's where mermaid steps in. Even GitHub can render mermaid diagrams native in markdown files.

src/GMCPAuthenticator.cpp

Co-authored-by: Marco Fontani <mf@marcofontani.it>

demonnic

Nothing in the code stands out as a bad idea, though you may want to wait for at least one review from a more practiced C++ coder than myself. Mudlet's and my own tests pass though so I'm giving it my thumbs up. 👍🏻

SlySven

I've only looked through the code on a C++ basis - whether it works or not is not something I can judge. I am not offering anything in regards to UI or Infrastructure "approval"...

src/GMCPAuthenticator.cpp

SlySven · 2024-03-25T19:25:44Z

src/GMCPAuthenticator.h

+ *   59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.             *
+ ***************************************************************************/
+
+#pragma once


That may be the way of things nowadays but since we have not adopted it elsewhere, why not stick with the existing include-guards methodology?

SlySven · 2024-03-25T19:27:38Z

src/GMCPAuthenticator.h

+#include "post_guard.h"
+
+
+class GMCPAuthenticator : public QObject


🤔 Is this class doing anything that requires it to be derived from the QObject class?

Yes, tr()

Well, in that case you should use Q_DECLARE_TR_FUNCTIONS(GMCPAuthenticator) (see Q_DECLARE_TR_FUNCTIONS) at the top of the class declaration in the header file instead of the Q_OBJECT macro. For example - as used in the TRoom class.

Why? this setup works.

The alternative I gave is a "lighter-weight" one - which we(I) have used elsewhere in the project.

This is premature optimisation for a class that's instantiated one per host... https://users.ece.utexas.edu/~adnan/pike.html. I don't think this is something we should be spending energy on.

SlySven · 2024-03-25T19:28:26Z

src/Host.cpp

 #include "TLabel.h"
 #include "TMap.h"
 #include "TMedia.h"
+#include "GMCPAuthenticator.h"


Sorted into the right place? Maybe not!

Co-authored-by: Stephen Lyons <slysven@virginmedia.com>

RahjIII · 2024-03-27T04:58:39Z

Mudlet 4.17.2-testing-pr7152-476efbbc seems to be working out just fine with GMCP Authentication. Its the fastest I've ever seen Mudlet log into anything. Very nice work! However...

I had originally set my game server to send a Char.Login.Result { "success": false, "message": "Bad GMCP Login Credentials." } message, and then "hang up" the connection, in the event of a good account name and bad password combination.

I had the "automatically reconnect" flag set in the profile... and Mudlet proceeded to repeatedly and rapidly reconnect and fail over and over again, at a rate of about seven attempts per second.

I think it would be a very good idea to have Mudlet break the "automatically reconnect" loop upon receipt of a Char.Login.Result failure message.

vadi2 · 2024-03-29T13:36:59Z

We will consider such speed in Mudlets behalf to be good :)

I have adjusted it not to reconnect upon authentication failure.

mfontani · 2024-03-29T13:39:45Z

We will consider such speed in Mudlets behalf to be good :)

Indeed!

I had the same problem, and had about 8.4 reconnections/s (on localhost) on password failure without this patch... but to test it I had to explicity tick the reconnection tickbox, which doesn't seem to be the default - phew!

I have adjusted it not to reconnect upon authentication failure.

Sounds very good, thanks!

src/GMCPAuthenticator.cpp

mpconley · 2024-03-30T11:55:10Z

src/CMakeLists.txt

+    FontManager.cpp
    GifTracker.cpp
-    TrailingWhitespaceMarker.cpp
+    GMCPAuthenticator.cpp


Are we doing away with the convention of putting a 'T' in front of our classes? Does it have a meaning?

For the meaning I am not sure but the T prefix could stand for a "template" (but not a C++ template) for a class that isn't associated with a dialogue (those begin with dlg instead!) Incidentally when describing a container or method for an arbitrary class T on it's own is often used as the identifier of the class. E.g. (bool) QListIterator<T>::hasNext() to test to see whether an iterator into a QList<T> has a value it can get. So maybe appending things to that seemed logical, QListIterator<TMapLabel>::hasNext() when iterating though a QList<TMapLabel>?

I don't see a reason continue tbh. I don't see a meaning behind it other than a Template which we don't use often.

SlySven

Well the changes I've suggested have pretty much been applied - apart from the one about the (better IMHO) alternative to the QOBJECT macro... once that has been done I am happy to approve it from a C++ POV. @demonnic seems to be happy with it. I dunno whether @Kebap has anything to say but I think this is getting close to being ready to ...

mpconley · 2024-04-06T18:42:13Z

Thank you for the workaround for the booleans. This is working out as expected now.

mpconley · 2024-04-07T13:07:25Z

Good to merge now? Let's do this? :)

-Tamarindo

vadi2 · 2024-04-07T14:11:38Z

Done ✅

Extended the functions disabled by the login timeout to include the `sendCharacterName()` function - and allow the timer that provides that timeout to be stopped by the GMCP authenticator that Mudlet#7152 introduced. Such a stop also immediately disables those functions so that it will neutralise any scripts or triggers that would be using them. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Vadim Peretokin added 7 commits February 17, 2024 09:17

Add GMCPAuthenticator class and handle authentication GMCP messages

15ca4e2

Start handling GMCP authentication

cc3bcec

Refactor GMCP authentication in cTelnet class

4dc7d0c

Refactor GMCPAuthenticator class and update license information

315975a

Add GMCPAuthenticator to Host class

43f0d37

Add GMCPAuthenticator class to Host.cpp and cTelnet.cpp

79d3cc8

Plug in implementation

971d901

vadi2 requested a review from a team February 17, 2024 11:07

vadi2 requested a review from a team as a code owner February 17, 2024 11:07

vadi2 requested a review from a team February 17, 2024 11:07

Merge branch 'development' into add-gmcp-authenticate

58bc908

Vadim Peretokin added 3 commits February 17, 2024 12:08

Undo erroneous cmake change

0f3e93e

Merge branch 'add-gmcp-authenticate' of https://github.com/vadi2/Mudlet…

a2fa918

… into add-gmcp-authenticate

Fix GMCPAuthenticator credentials encoding

1070c59

vadi2 changed the title ~~Add: GMCP authenticate spec~~ Add: GMCP authenticate spec implementation Feb 17, 2024

Vadim Peretokin and others added 6 commits February 17, 2024 12:26

Code review fixes

09e11d9

Merge branch 'development' into add-gmcp-authenticate

6988060

Merge branch 'development' into add-gmcp-authenticate

e81194a

Update type key to be spec compliant

1c5c418

Add login timer cancellation

3d581f1

Merge branch 'add-gmcp-authenticate' of https://github.com/vadi2/Mudlet…

ade1895

… into add-gmcp-authenticate

vadi2 mentioned this pull request Feb 18, 2024

Improve: add functions for handling game character details #3952

Closed

vadi2 and others added 3 commits February 18, 2024 20:58

Merge branch 'development' into add-gmcp-authenticate

db8084a

Re-add missing IAC SB GMCP

4254562

Merge branch 'add-gmcp-authenticate' of https://github.com/vadi2/Mudlet…

102bc6d

… into add-gmcp-authenticate

Merge branch 'development' into add-gmcp-authenticate

e743481

vadi2 added 2 commits March 3, 2024 11:57

Merge branch 'development' into add-gmcp-authenticate

aef183d

Add logging for GMCP credentials

44a395c

mfontani reviewed Mar 13, 2024

View reviewed changes

src/GMCPAuthenticator.cpp Show resolved Hide resolved

vadi2 and others added 4 commits March 13, 2024 09:30

Update src/GMCPAuthenticator.cpp

ca1ad62

Co-authored-by: Marco Fontani <mf@marcofontani.it>

Merge branch 'development' into add-gmcp-authenticate

1c08422

Print warn for invalid credentials

1241392

Merge branch 'development' into add-gmcp-authenticate

476efbb

demonnic approved these changes Mar 25, 2024

View reviewed changes

SlySven reviewed Mar 25, 2024

View reviewed changes

vadi2 and others added 3 commits March 25, 2024 21:34

Apply suggestions from code review

ddd1b63

Co-authored-by: Stephen Lyons <slysven@virginmedia.com>

Update src/GMCPAuthenticator.cpp

bdd4af0

Co-authored-by: Stephen Lyons <slysven@virginmedia.com>

Wrap all in qDebug

7b030b5

Vadim Peretokin added 3 commits March 29, 2024 14:16

Review feedback

3a487e2

sort host includes

484b1c9

Don't reconnect if authentication failed

96de847

mpconley reviewed Mar 29, 2024

View reviewed changes

src/GMCPAuthenticator.cpp Outdated Show resolved Hide resolved

mpconley reviewed Mar 30, 2024

View reviewed changes

SlySven approved these changes Apr 5, 2024

View reviewed changes

vadi2 added 2 commits April 6, 2024 18:38

Review feedback

5856667

Add workaround for booleans

3882561

vadi2 merged commit 0ac53a3 into Mudlet:development Apr 7, 2024

vadi2 deleted the add-gmcp-authenticate branch April 7, 2024 14:11

		#include "post_guard.h"


		class GMCPAuthenticator : public QObject

Uh oh!

Conversation

vadi2 commented Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Brief overview of PR changes/additions

Motivation for adding to Mudlet

Other info (issues closed, discussion etc)

Uh oh!

add-deployment-links bot commented Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

mpconley commented Feb 25, 2024

Uh oh!

vadi2 commented Feb 25, 2024

Uh oh!

vadi2 commented Feb 25, 2024

Uh oh!

vadi2 commented Mar 1, 2024

Uh oh!

Uh oh!

demonnic left a comment

Choose a reason for hiding this comment

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RahjIII commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadi2 commented Mar 29, 2024

Uh oh!

mfontani commented Mar 29, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 Apr 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

mpconley commented Apr 6, 2024

Uh oh!

mpconley commented Apr 7, 2024

Uh oh!

vadi2 commented Apr 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

vadi2 commented Feb 17, 2024 •

edited

Loading

add-deployment-links bot commented Feb 17, 2024 •

edited

Loading

SlySven Mar 25, 2024 •

edited

Loading

RahjIII commented Mar 27, 2024 •

edited

Loading

vadi2 Apr 6, 2024 •

edited

Loading