Topic: Add player related functions to nettool (work in progress)

[Ashley]

This patch adds some functions to nettool, it's still a work in progress however...

Functions added:

Code: [Select]

      players
        Receive list of player information from server

      player-name <player number> <new name>
        Change the name of a player

      player-pass <player number> <new password>
        Change the password of a player

I also considered adding a "reset-player" command, which would delete a company from the game. But I am not sure if this is really a useful feature or not.

The player list is returned in a crude CSV form. I think it's better to return data from the game in either CSV or another form (CSV is very simple and easy to implement, unlike XML...) rather than passing back serialised native objects which the nettool does not need to know about. I'm planning on converting the other functions of nettool to a similar scheme. Then there can be a simple switch to make nettool print either the raw CSV data (useful for other scripts/utilities to print) or pretty-printing the CSV data for human consumption.

This approach makes it a lot easier to add functions to retrieve data.

Next step (underway) is writing a simple CSV parsing class for use by nettool and also in transferring things like the game listings from the listings server (to promote code re-use).

I also need to find a way to lookup the client ID of the client currently playing as each player.

[jamespetts]

These features will be very useful, I think. Not being able to reset a player's password can cause real difficulties in administration either with players who have dropped out or with players who have forgotten their passwords.

[Dwachs]

Not being able to reset a player's password can cause real difficulties in administration either with players who have dropped out or with players who have forgotten their passwords.

Any human, who has public player unlocked, can change passwords of any player.

[Ashley]

True Dwachs, but a big motivation for features in nettool (for me) is not having to actually open a copy of Simutrans in order to administer the server. This also allows automation, e.g. ensuring that the human and public service players are locked when a new map is loaded (and renaming human to spectator).

I just noticed that I ought to hash the password before sending it to the server. Also the game admin password is sent in clear text too, so I'll change both to use hashes.

[isidoro]

[...]
I just noticed that I ought to hash the password before sending it to the server. Also the game admin password is sent in clear text too, so I'll change both to use hashes.

I don't understand this, but maybe I'm thick today.  What difference would it make?  An attacker can send the hash he spied upon before...  Remember that anyone can compile a modified version of ST...

[Ashley]

Good point, I am just allergic to sending passwords in plaintext. I only use the nettool to connect to the server on localhost anyway (others may use it in other ways). Another enhancement is to add a whitelist of IP addresses from which the server will permit nettool connections (so you can restrict it to localhost only). This I will do along with the blacklist enhancements, since it is basically the same thing.

[prissi]

A password may be used in other places. And simutrans can salt password with something not so easy to predict, like the map number/heighmap/current year. In that case sending hashes will make a difference, as one would need to know the time and savegame.

[Ashley]

Another work in progress, simple CSV class, permits field-by-field building of CSV data, as well as reading through the contained data field-by-field as well. Depends on the cbuffer_t class for internal storage. There's a short test program as well (stick these all under /utils/ and use the Makefile to compile).

This will be useful to replace the server_frame_t::parse_csv_field method (which is less general purpose), and I plan on using it more extensively in nettool as well.

Still some bugs to work out and finish off though!

[Ashley]

Still working on this.

csv_patch_2.patch
This adds a simple CSV encoding/decoding library based around a CSV_t object, backed by a cbuffer_t. It also adds an overloaded append() method to the cbuffer_t to let you specify a maximum length to copy.

network_mem.patch
This adds an HTTP GET method which instead of receiving data to a file on disk, receives it instead into a cbuffer_t. This avoids having to write the network game listing to disk and then read it back in again. Similarly it adds a network_receive_data method which receives into a cbuffer_t.

server_frame_csv.patch
This switches the server frame (netgame dialog) to use the new CSV library instead of the existing class method. It also switches it to use the new memory-based HTTP method.

(All three patches are not final, and need further testing!)


The CSV library will then be used for the nettool as well, to provide a consistent data interchange method between Simutrans and nettool (with either pretty-printed formatted output, or CSV output to allow easy parsing by external tools).

[Ashley]

Why does cbuffer_t have private copy constructor and assignment operator overload? Obviously this prevents making copies (and passing by value) in the obvious way, but is there a reason for this?

[Dwachs]

This is a consequence of the discussion here:

http://forum.simutrans.com/index.php?topic=8994.0

Ie the cbuffer class has a custom constructor & destructor, so the copy-constructor and copy-assignment operator need a custom implementation, too. For now these methods are declared private to prevent the compiler from using default methods. You can however implement these functions (both of them) and then use it.

You can also pass this class by reference

Code: [Select]

void something(cbuffer_t const& buf)
Passing by value would require a lot of redundant copy operations.

[Ashley]

Ok, I'll implement copy-constructor and assignment operator for cbuffer_t, since it makes the implementation of CSV_t a lot cleaner (since that then doesn't need destructor, copy-constructor and assignment operator itself...)

I am passing cbuffer_t objects by reference, they will only get copied if the CSV_t itself gets copied.

[Ashley]

Ok I think this is done, patches should apply against r5477. I've tested the CSV library with a test program and it seems pretty stable (will probably find some more bugs as I use it more for nettool).

If these patches are satisfactory, could they be merged, since it makes it easier to keep track of things if the changes are in the trunk

[Dwachs]

Where do you need the copy constructor / assignment ?

Why did you implement another network_receive_method ? Why did you not use network_receive_data?

I also would like to change the csv_T::get_field method to something like

Code: [Select]

int get_next_field (cbuffer_t &buf);


[Ashley]

The cbuffer_t copy constructor/assignment operator would be used if a CSV_t object were to be copied. It avoids having to implement a copy constructor/destructor/assignment operator overload for the CSV_t class.

I could change it to use network_receive_data instead, although then there's no progress bar.

Edit: Also using network_receive_data means having to allocate memory twice, since you'd need to allocate a buffer to receive the data into and then copy the data from that buffer into the cbuffer_t passed into network_http_get().

That last one is easy enough to do. Why are we using cbuffer_t anyway and not std::string?

[Ashley]

Here are patches with the requested changes made

[Dwachs]

Thanks! These are now incorporated in r5508.

What is the state of the patch in the first post?

[Ashley]

Working on that now Shouldn't be too much work with these other bits finished.

[prissi]

The hashing of password is aparently still omitted: Why? We should still avoid sending clear text password, as user might user their password elsewhere.

[Ashley]

I haven't got to that bit yet, the accepted patches don't deal with nettool, the nettool patch will include hashing of the passwords before sending them over the wire.