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

Client identity storage service #2965

Merged
merged 17 commits into from Jun 19, 2017

Conversation

Projects
5 participants
@gianluca-nitti
Member

gianluca-nitti commented May 29, 2017

Contains

The client to synchronize with a client identity storage service, as discussed here in the forum.

How to test

An implementation of the backend server is available here. You can install and configure it locally as described here, or use the public instance I'm hosting here.

To test, register an account on a server by navigating to it's address with a browser (or directly clicking the link above if you use the hosted instance). Then, start the game client (from this branch) and login by going to Settings > Player Settings. Now you can join a game server and as soon as you do, the new identity will be uploaded to the service; you can then start another client with empty/different configuration (e.g. use the 2nd or 3rd client run configurations in IntelliJ, or specify another data directory from the command line) and login into it too; your client identities will be synchronized and if you join the same game server, you will find your character in the same position you left it from the other client.

Outstanding before merging

Marking as WIP since I think there is still some minor improvement that can be done, e.g. localization of some UI messages. Also, the conflict resolving feature (which allows you which identity you want to keep for a certain server in case the locally stored one conflicts with the one stored on the service) is not fully working but this is due to a server issue which doesn't allow to overwrite the identities (will fix it soon).

Update: I just added the localization feature (the strings are in the menu translations files; the values are filled for English only but can be easily be added for the other languages via editing the proper files or - I think - with Weblate if this branch is merged). Also fixed the server to support replacing of already uploaded identities.

Tagging @msteiger since we talked about this recently on appear.in.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 29, 2017

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 29, 2017

Hooray Jenkins reported success with all tests good!

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 30, 2017

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 30, 2017

Hooray Jenkins reported success with all tests good!

@gianluca-nitti gianluca-nitti added this to In progress in GSOC 2017 - Server API May 30, 2017

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 31, 2017

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 31, 2017

Hooray Jenkins reported success with all tests good!

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 31, 2017

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 31, 2017

Hooray Jenkins reported success with all tests good!

@gianluca-nitti gianluca-nitti changed the title from [WIP] Client identity storage service to Client identity storage service May 31, 2017

@msteiger

Very good PR! Just a few minor things:

Class names should be camel case: GET -> Get and POST -> Post
No wildcard imports, please.

Show outdated Hide outdated engine/src/main/java/org/terasology/identity/ClientIdentity.java
@Override
public int hashCode() {
return playerPrivateCertificate.hashCode() ^ playerPrivateCertificate.hashCode();

This comment has been minimized.

@msteiger

msteiger Jun 2, 2017

Member

I think that this will always be zero. Maybe just just 31 * playerPublicCertificate instead?

@msteiger

msteiger Jun 2, 2017

Member

I think that this will always be zero. Maybe just just 31 * playerPublicCertificate instead?

This comment has been minimized.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Wanted to XOR the public with the private but accidentally typed private two times. I just found there is an Objects.hash for this purpose of building hashcodes for objects composed by multiple fields, would that be OK? Will change again if you prefer the manual implementation.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Wanted to XOR the public with the private but accidentally typed private two times. I just found there is an Objects.hash for this purpose of building hashcodes for objects composed by multiple fields, would that be OK? Will change again if you prefer the manual implementation.

This comment has been minimized.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Also changed the hashCode() implementation in PrivateIdentityCertificate to use this method. I'm not sure if I should change it in PlayerPublicCertificate too since an implementation was already there (I didn't added it in this PR) which simply returns signature.hashCode(); combining with the other fields (id, modulus and exponent) will probably reduce the collision rate but could also slow down the computation of the hash, what do you think about it?

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Also changed the hashCode() implementation in PrivateIdentityCertificate to use this method. I'm not sure if I should change it in PlayerPublicCertificate too since an implementation was already there (I didn't added it in this PR) which simply returns signature.hashCode(); combining with the other fields (id, modulus and exponent) will probably reduce the collision rate but could also slow down the computation of the hash, what do you think about it?

Show outdated Hide outdated ...e/src/main/java/org/terasology/identity/storageServiceClient/Action.java
/**
* Represents an interaction that can be made with the storage service server.
*/
abstract class Action {

This comment has been minimized.

@msteiger

msteiger Jun 2, 2017

Member

Either make it an interface or use Consumer<StorageServiceWorker> instead.

@msteiger

msteiger Jun 2, 2017

Member

Either make it an interface or use Consumer<StorageServiceWorker> instead.

This comment has been minimized.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Interesting point, making it an interface. In both cases I need to make the method public, however shouldn't be a big deal since both the interface and all the implementor classes are package-private and references to those classes never "leak" outside the package, right?

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Interesting point, making it an interface. In both cases I need to make the method public, however shouldn't be a big deal since both the interface and all the implementor classes are package-private and references to those classes never "leak" outside the package, right?

@Override
public BigInteger deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException {
return new BigInteger(DECODER.decode(json.getAsString().replace("\n", "")));

This comment has been minimized.

@msteiger

msteiger Jun 2, 2017

Member

Why could there be newline chars. in the data?

@msteiger

msteiger Jun 2, 2017

Member

Why could there be newline chars. in the data?

This comment has been minimized.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

I know it looks ugly, the problem is that the PostgreSQL base64 encoder used on the server inserts some newlines every a certain numbers of chars. I'll see if I can strip them on the server instead.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

I know it looks ugly, the problem is that the PostgreSQL base64 encoder used on the server inserts some newlines every a certain numbers of chars. I'll see if I can strip them on the server instead.

This comment has been minimized.

@msteiger

msteiger Jun 5, 2017

Member

Nevermind

@msteiger

msteiger Jun 5, 2017

Member

Nevermind

Show outdated Hide outdated ...c/main/java/org/terasology/identity/storageServiceClient/HttpMethod.java
/**
* Http methods; only the relevant ones for now but can easily be extended with the other standard ones.
*/
enum HttpMethod {

This comment has been minimized.

@msteiger

msteiger Jun 2, 2017

Member

Maybe complete the list and make it public? There are not so many HTTP Methods

@msteiger

msteiger Jun 2, 2017

Member

Maybe complete the list and make it public? There are not so many HTTP Methods

This comment has been minimized.

Show outdated Hide outdated ...java/org/terasology/identity/storageServiceClient/ServiceAPIRequest.java
*/
final class ServiceAPIRequest {
private static final Gson GSON = new GsonBuilder().registerTypeHierarchyAdapter(BigInteger.class, BigIntegerBase64Serializer.getInstance()).create();

This comment has been minimized.

@msteiger

msteiger Jun 2, 2017

Member

Can you reuse one of the existing instances of GSON? Not sure if it's possible or makes sense ..

@msteiger

msteiger Jun 2, 2017

Member

Can you reuse one of the existing instances of GSON? Not sure if it's possible or makes sense ..

This comment has been minimized.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

I don't think so, I searched for all the usages of GsonBuilder in the project and looks like that all the classes which need JSON serialization make their own instance with the appropriate type adapters.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

I don't think so, I searched for all the usages of GsonBuilder in the project and looks like that all the classes which need JSON serialization make their own instance with the appropriate type adapters.

}
status = StorageServiceWorkerStatus.WORKING;
logger.info("Performing action {}", action.getClass().getSimpleName());
new Thread(() -> {

This comment has been minimized.

@msteiger

msteiger Jun 2, 2017

Member

Please provide a Runnable to the Thread instead of subclassing it. I hope we have a shared thread pool executor soon, so you can just throw it there.

@msteiger

msteiger Jun 2, 2017

Member

Please provide a Runnable to the Thread instead of subclassing it. I hope we have a shared thread pool executor soon, so you can just throw it there.

This comment has been minimized.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Please forgive me but I don't understand what you mean here, I'm providing an anonymous implementation of Runnable to the Thread's constructor, not subclassing Thread.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Please forgive me but I don't understand what you mean here, I'm providing an anonymous implementation of Runnable to the Thread's constructor, not subclassing Thread.

This comment has been minimized.

@msteiger

msteiger Jun 5, 2017

Member

Ah sorry, I misread.

@msteiger

msteiger Jun 5, 2017

Member

Ah sorry, I misread.

Show outdated Hide outdated ...terasology/identity/storageServiceClient/StorageServiceWorkerStatus.java
return buttonEnabled;
}
public String getLocalizedStatusMessage(TranslationSystem translationSystem, String loginName) {

This comment has been minimized.

@msteiger

msteiger Jun 2, 2017

Member

Those two methods look strange to me. I would move it either to a static helper class or - even better - to the (single?) caller class.

@msteiger

msteiger Jun 2, 2017

Member

Those two methods look strange to me. I would move it either to a static helper class or - even better - to the (single?) caller class.

This comment has been minimized.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Moving to static helper class since getLocalizedStatusMessage is called in two places (PlayerSettingsScreen and MainMenuScreen)

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Moving to static helper class since getLocalizedStatusMessage is called in two places (PlayerSettingsScreen and MainMenuScreen)

Show outdated Hide outdated engine/src/main/java/org/terasology/logic/console/ConsoleSystem.java
import org.terasology.network.ClientComponent;
import org.terasology.registry.In;
import org.terasology.rendering.nui.NUIManager;
@RegisterSystem
public class ConsoleSystem extends BaseComponentSystem {
private static final ResourceUrn MINICHAT_UI = new ResourceUrn("engine:minichatOverlay");

This comment has been minimized.

@msteiger

msteiger Jun 2, 2017

Member

Why is that here now?

@msteiger

msteiger Jun 2, 2017

Member

Why is that here now?

This comment has been minimized.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

I used the MiniChatOverlay to show notifications from the storage service in the menus. If I understand correctly in the menu state there is no ChatSystem so I made some modifications to ConsoleSystem to show certain notifications in MiniChatOverlay. In my opinion it's a good way to notify user when, for example, the synchronization is completed - since it happens asynchronously, I thought that showing message boxes may not be a good idea since they would pop up at random moments possibly annoying the user.
Let me now if something needs to be changed.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

I used the MiniChatOverlay to show notifications from the storage service in the menus. If I understand correctly in the menu state there is no ChatSystem so I made some modifications to ConsoleSystem to show certain notifications in MiniChatOverlay. In my opinion it's a good way to notify user when, for example, the synchronization is completed - since it happens asynchronously, I thought that showing message boxes may not be a good idea since they would pop up at random moments possibly annoying the user.
Let me now if something needs to be changed.

This comment has been minimized.

@msteiger

msteiger Jun 5, 2017

Member

That's an interesting idea - I like it. The word "chat" could be confusing though. Maybe we can find a better name... NotificationOverlay maybe? Does not need to happen in this PR though.

@msteiger

msteiger Jun 5, 2017

Member

That's an interesting idea - I like it. The word "chat" could be confusing though. Maybe we can find a better name... NotificationOverlay maybe? Does not need to happen in this PR though.

This comment has been minimized.

@gianluca-nitti

gianluca-nitti Jun 5, 2017

Member

I agree, the name "chat" would not be consistent anymore. I did the refactoring anyway in the latest commit since it was just a 5 minute task.

@gianluca-nitti

gianluca-nitti Jun 5, 2017

Member

I agree, the name "chat" would not be consistent anymore. I did the refactoring anyway in the latest commit since it was just a 5 minute task.

@@ -82,6 +82,9 @@
@LayoutConfig
protected boolean readOnly;
@LayoutConfig
private boolean passwordMode;

This comment has been minimized.

@msteiger

msteiger Jun 2, 2017

Member

Many UI frameworks have a separate class (derived from the default text control). Have you thought about creating a UIPasswordText instead? It could be too much duplication, though ..

@msteiger

msteiger Jun 2, 2017

Member

Many UI frameworks have a separate class (derived from the default text control). Have you thought about creating a UIPasswordText instead? It could be too much duplication, though ..

This comment has been minimized.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Yes considered that, but avoided for

too much duplication

exactly this reason. Can move to this approach if you prefer, though.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Yes considered that, but avoided for

too much duplication

exactly this reason. Can move to this approach if you prefer, though.

This comment has been minimized.

@msteiger

msteiger Jun 5, 2017

Member

Ok, fair enough. Thanks for clarifying

@msteiger

msteiger Jun 5, 2017

Member

Ok, fair enough. Thanks for clarifying

@@ -177,6 +180,15 @@ public UIText(String id) {
cursorTexture = Assets.getTexture("engine:white").get();
}
private String buildPasswordString() {

This comment has been minimized.

@msteiger

msteiger Jun 2, 2017

Member

You could use this snippet if you like

char[] arr = new char[33];
Arrays.fill(arr, '*');
String str = String.valueOf(arr);
@msteiger

msteiger Jun 2, 2017

Member

You could use this snippet if you like

char[] arr = new char[33];
Arrays.fill(arr, '*');
String str = String.valueOf(arr);

This comment has been minimized.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

I was trying to figure out a better approach than using the StringBuilder but nothing came to my mind. Will now replace with your snippet, thanks.

@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

I was trying to figure out a better approach than using the StringBuilder but nothing came to my mind. Will now replace with your snippet, thanks.

@gianluca-nitti

This comment has been minimized.

Show comment
Hide comment
@gianluca-nitti

gianluca-nitti Jun 2, 2017

Member

Thank you for the review! Left some comments and made a commit with various fixes.

Member

gianluca-nitti commented Jun 2, 2017

Thank you for the review! Left some comments and made a commit with various fixes.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 2, 2017

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 2, 2017

Hooray Jenkins reported success with all tests good!

@gianluca-nitti gianluca-nitti moved this from In progress to Pull requests in review in GSOC 2017 - Server API Jun 2, 2017

gianluca-nitti added a commit to gianluca-nitti/Terasology that referenced this pull request Jun 5, 2017

Rename minichatOverlay to notificationOverlay
Reason: the changes proposed in #2965 use this widget to show messages from the identity storage service too.
@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 5, 2017

Member

Uh oh, something went wrong with the build. Need to check on that

Member

GooeyHub commented Jun 5, 2017

Uh oh, something went wrong with the build. Need to check on that

Rename minichatOverlay to notificationOverlay
Reason: the changes proposed in #2965 use this widget to show messages from the identity storage service too.
@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 5, 2017

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 5, 2017

Hooray Jenkins reported success with all tests good!

Move StorageServiceWorker to rootContext
Previously, the instance of this class was being registered in the main menu state's context, leading to NullPointerException when trying to access the player settings screen from the in-game pause-menu.
@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 6, 2017

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 6, 2017

Hooray Jenkins reported success with all tests good!

@gianluca-nitti

This comment has been minimized.

Show comment
Hide comment
@gianluca-nitti

gianluca-nitti Jun 6, 2017

Member

I added one more commit to fix a problem: trying to open the player's settings screen from the in-game pause menu was throwing a NullReferenceException due to the StorageServiceWorker instance (which the player settings screen polls) was being registered in the main menu's context, and thus not accessible from the in-game context.

I apologize if there is something bad in the fact that I added code that nobody asked for after the PR had been approved, possibly making the review/merge process longer, but I think this is still better than letting buggy code to propagate to other branches (develop/master). Unfortunately I didn't noticed this behavior when I tested before opening the PR because I didn't think about opening the settings from the pause menu.

Anyway, now it's fixed: the StorageServiceWorker instance is registered in the rootContext and thus is more decoupled from engine and user interface's states, and the player settings screen can be accessed both from the main menu and from the in-game pause menu.

Member

gianluca-nitti commented Jun 6, 2017

I added one more commit to fix a problem: trying to open the player's settings screen from the in-game pause menu was throwing a NullReferenceException due to the StorageServiceWorker instance (which the player settings screen polls) was being registered in the main menu's context, and thus not accessible from the in-game context.

I apologize if there is something bad in the fact that I added code that nobody asked for after the PR had been approved, possibly making the review/merge process longer, but I think this is still better than letting buggy code to propagate to other branches (develop/master). Unfortunately I didn't noticed this behavior when I tested before opening the PR because I didn't think about opening the settings from the pause menu.

Anyway, now it's fixed: the StorageServiceWorker instance is registered in the rootContext and thus is more decoupled from engine and user interface's states, and the player settings screen can be accessed both from the main menu and from the in-game pause menu.

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Jun 9, 2017

Member

Tried testing this out, but the server you're hosting seemingly took my registration (again I guess, tried earlier but the site looks much better now) then never sent me a confirmation key. Is it still working? :-)

Also wondering where we should host it. Since our Weblate instance runs out of Docker already maybe we could just add a couple more containers or something? Poking @qwc and @msteiger for ideas

Game itself seemed to run fine and I was able to test it successfully in the past!

Member

Cervator commented Jun 9, 2017

Tried testing this out, but the server you're hosting seemingly took my registration (again I guess, tried earlier but the site looks much better now) then never sent me a confirmation key. Is it still working? :-)

Also wondering where we should host it. Since our Weblate instance runs out of Docker already maybe we could just add a couple more containers or something? Poking @qwc and @msteiger for ideas

Game itself seemed to run fine and I was able to test it successfully in the past!

@gianluca-nitti

This comment has been minimized.

Show comment
Hide comment
@gianluca-nitti

gianluca-nitti Jun 9, 2017

Member

@Cervator : it's OK that it allowed you to "register again" after the previous time you registered because I rebuilt the database about a week ago when I added email verification and reCAPTCHA (the only accounts were mine and your tests, so I guessed it wasn't a big deal; anyway, I have a dump). I had a look at the database now and saw you made an account with email address which is waiting for confirmation.

I tried to register another test account with my email and it worked, I got the activation mail - in the spam folder, however. Did you check yours? The reason it goes into spam, I guess, is that it doesn't come from a well-known/trusted mail service, but from a local SMTP server installed on the VPS, so Gmail thinks it's suspicious.
P.S. even if you find the "old" verification email in spam, if you want to retry you probably need to redo the registration (same username and email is fine) because an unverified account expires after 1-2 hours (not sure about the exact value now).

Member

gianluca-nitti commented Jun 9, 2017

@Cervator : it's OK that it allowed you to "register again" after the previous time you registered because I rebuilt the database about a week ago when I added email verification and reCAPTCHA (the only accounts were mine and your tests, so I guessed it wasn't a big deal; anyway, I have a dump). I had a look at the database now and saw you made an account with email address which is waiting for confirmation.

I tried to register another test account with my email and it worked, I got the activation mail - in the spam folder, however. Did you check yours? The reason it goes into spam, I guess, is that it doesn't come from a well-known/trusted mail service, but from a local SMTP server installed on the VPS, so Gmail thinks it's suspicious.
P.S. even if you find the "old" verification email in spam, if you want to retry you probably need to redo the registration (same username and email is fine) because an unverified account expires after 1-2 hours (not sure about the exact value now).

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger Jun 18, 2017

Member

Can we merge this one, @Cervator?

Member

msteiger commented Jun 18, 2017

Can we merge this one, @Cervator?

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Jun 19, 2017

Member

@msteiger certainly - checked it out again and rested, got the confirmation email via junk mail, confirmed, and was able to validate the identity carried over :-) You don't need to wait for me if you have approved and feel you've tested decently, in that case merge ahead and just assign the PR (and any related issues) to the current milestone

Great work @gianluca-nitti !

One small suggestion: Make the UI element a little more consistent between different screens - the little thing in the top right is only visible on the main menu? Maybe it should be there on the settings screen etc too?

We should probably also find some places to update the docs, and a good on-going way to host the service. Will bump the forum thread :-)

Thanks again!

Member

Cervator commented Jun 19, 2017

@msteiger certainly - checked it out again and rested, got the confirmation email via junk mail, confirmed, and was able to validate the identity carried over :-) You don't need to wait for me if you have approved and feel you've tested decently, in that case merge ahead and just assign the PR (and any related issues) to the current milestone

Great work @gianluca-nitti !

One small suggestion: Make the UI element a little more consistent between different screens - the little thing in the top right is only visible on the main menu? Maybe it should be there on the settings screen etc too?

We should probably also find some places to update the docs, and a good on-going way to host the service. Will bump the forum thread :-)

Thanks again!

@Cervator Cervator merged commit 0927338 into MovingBlocks:develop Jun 19, 2017

1 check passed

default Build finished.
Details

@Cervator Cervator added this to the Alpha 8 milestone Jun 19, 2017

@qwc

This comment has been minimized.

Show comment
Hide comment
@qwc

qwc Jun 19, 2017

@Cervator Of course, spinning up docker containers is the easiest. That's the fancy thing with docker.
But we should think twice regarding resources usage, the weblate droplet may be to small for things like elastic search, etc.

qwc commented Jun 19, 2017

@Cervator Of course, spinning up docker containers is the easiest. That's the fancy thing with docker.
But we should think twice regarding resources usage, the weblate droplet may be to small for things like elastic search, etc.

@gianluca-nitti gianluca-nitti moved this from Pull requests in review to Merged/completed in GSOC 2017 - Server API Jun 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment