anonymous auth for mobile #660

jessicafalk · 2018-12-20T14:33:24Z

Description

Added anonymous functionality to the workerconnector.
Adjusted scenes to allow to choose between connecting locally or via cloud. we have now 2 buttons to select whether you want to connect to a local or a cloud deployment

Tests

Tested on both iOS and Android. Works as expected

Documentation

docs will follow in a separate PR

Primary reviewers

If your change will take a long time to review, you can name at most two primary reviewers who are ultimately responsible for reviewing this request. @ mention them.

jared-improbable · 2018-12-20T14:44:11Z

workers/unity/Packages/com.improbable.gdk.core/Worker/WorkerConnector.cs

@@ -87,6 +87,7 @@ public async Task Connect(string workerType, ILogDispatcher logger)
                var origin = transform.position;
                ConnectionDelegate connectionDelegate;
                var chosenService = GetConnectionService();
+                Debug.Log(chosenService);


Remove debug print

jared-improbable · 2018-12-20T14:46:43Z

workers/unity/Packages/com.improbable.gdk.mobile/Worker/MobileWorkerConnector.cs

+
+            if (!string.IsNullOrEmpty(pit.Error))
+            {
+                throw new ConnectionFailedException(pit.Error, ConnectionErrorReason.InvalidConfig);


Is InvalidConfig actually a valid reason for failure in every case, here and below?
What if it's a different reason, for example if the service is down?

jared-improbable · 2018-12-20T14:47:19Z

workers/unity/Packages/com.improbable.gdk.mobile/Worker/MobileWorkerConnector.cs

+                    ConnectionErrorReason.InvalidConfig);
+            }
+
+            if (!loginTokenRequestResult.Value.Status.Equals(ConnectionStatusCode.Success))


Why ! .Equals, rather than !=?

gkassabli

Looks good overall!

gkassabli · 2019-01-07T17:50:12Z

spatialos.json

@@ -1,8 +1,8 @@
 {
    "name": "unity_gdk",
    "project_version": "0.0.1",
-    "sdk_version": "13.5.0-b7795-fdd6a-WORKER-SNAPSHOT",
+    "sdk_version": "13.5.0",


Does this not break our tracking?

as discussed internally: as long as we are not doing a new release it should be ok

Yup - ticket to track: https://improbableio.atlassian.net/browse/UTY-1580

gkassabli · 2019-01-07T17:54:11Z

workers/unity/Assets/Playground/Scripts/Worker/iOSClientWorkerConnector.cs

@@ -55,6 +59,26 @@ protected override string GetHostIp()
 #endif
        }

+        protected override string GetPlayerId()


Do we want to move this to MobileWorkerConnector to avoid duplication?

There's definitely some code duplication here (and in the Android one) that could be factored down to the parent class

yeah, agreed. on one hand, I want the user to define it, on the other hand, this is only needed in very specific cases, so the user shouldn't need to always define it... I might just move the default all the way up to the WorkerConnector until we can finally refactor this.

gkassabli · 2019-01-07T18:05:38Z

workers/unity/Packages/com.improbable.gdk.core/Worker/WorkerConnector.cs

+        {
+            var loginTokenRequestResult = DevelopmentAuthentication.CreateDevelopmentLoginTokensAsync(
+                RuntimeConfigDefaults.LocatorHost,
+                444,


DevelopmentPlayerIdentityTokenPort?

jamiebrynes7 · 2019-01-07T19:22:55Z

workers/unity/Assets/Playground/Scripts/Worker/AndroidClientWorkerConnector.cs


-        public async void TryConnect()
+        public async void TryConnect(ConnectionService connectionService)


I think @samiwh noted on this in another PR - but should async methods have async in the name. I.e. - TryConnectAsync

jamiebrynes7 · 2019-01-07T19:24:53Z

workers/unity/Assets/Playground/Scripts/Worker/iOSClientWorkerConnector.cs

@@ -55,6 +59,26 @@ protected override string GetHostIp()
 #endif
        }

+        protected override string GetPlayerId()


There's definitely some code duplication here (and in the Android one) that could be factored down to the parent class

jamiebrynes7 · 2019-01-07T19:27:29Z

workers/unity/Packages/com.improbable.gdk.core/Worker/WorkerConnector.cs

+        protected virtual string SelectLoginToken(List<LoginTokenDetails> loginTokens)
+        {
+            return null;
+        }


Need docstrings on the new protected virtual methods in the class

ah, knew I had forgotten something

jamiebrynes7 · 2019-01-07T19:30:27Z

workers/unity/Packages/com.improbable.gdk.core/Worker/WorkerConnector.cs

+        protected virtual string GetDisplayName()
+        {
+            return null;
+        }


I know this similar to other functions defined here - but I feel we should either provide a reasonable default implementation or make these fully abstract.

Returning null as a default implementation seems likely to break things later on.

workers/unity/Packages/com.improbable.gdk.core/Worker/WorkerConnector.cs

jamiebrynes7 · 2019-01-07T19:33:39Z

workers/unity/Packages/com.improbable.gdk.core/Worker/WorkerConnector.cs

+
+            if (loginTokenRequestResult.Value.Status != ConnectionStatusCode.Success)
+            {
+                throw new ConnectionFailedException($"Failed to retrieve login token, " +


I'd rather have a slightly longer line length than the string concatenation here.

Mild preference though

jamiebrynes7 · 2019-01-07T19:34:19Z

workers/unity/Packages/com.improbable.gdk.mobile/Android/Utility/LaunchArguments.cs

@@ -27,7 +27,7 @@ public static class LaunchArguments
            }
            catch (Exception e)
            {
-                UnityEngine.Debug.LogException(e);
+                UnityEngine.Debug.LogWarning($"Failed to retrieve launch arguments: {e}");


Exception -> Warning? What's the context here?

this will always fail, if you run this in the editor after switching to Android as current platform

therefore, it is expected behaviour to fail depending on what exactly you do and it shouldn't throw an exception imo

Should this function even be able to be called in the editor then if it will always fail?

fair point. we could also change the part where it's called to only get called if android and not editor. or wrap this part here around an #if UNITY_EDITOR, which I would slightly prefer as it makes it safe to call for users that are not familiar with the code.

jamiebrynes7 · 2019-01-08T09:40:11Z

Also changelog 😛

workers/unity/Packages/com.improbable.gdk.core/Exceptions/AuthenticationFailedException.cs

workers/unity/Packages/com.improbable.gdk.core/Worker/Worker.cs

…y into feature/anon-auth

gkassabli · 2019-01-08T21:12:51Z

workers/unity/Assets/Playground/Scripts/UI/ConnectionScreenController.cs

@@ -27,17 +28,20 @@ public void Awake()
            if (!string.IsNullOrEmpty(hostIp))
            {
                ipAddressInput.text = hostIp;
-                TryConnect();


Is this intentional change? If I understand correctly this was here to automatically connect to host locally when we launch with command line arguments

This is intentional, should have probably commented that earlier. Now that we can either connect to a local or a cloud deployment, we don‘t want to try and immediately connect to a local deployment all the time. Sure, we could remove the ip in the config, but I think this is a nicer solution as the other one would mean that you would have to look up for local ip again

Fine with me if this is explicit change.
I'd point out that this was only triggered from launch menu, where I'd expect people mostly launch for local, but I understand the reasoning.
I think this is another pointer that we need to rethink how we implement client launch configuration so that it is more transparent and processing logic is easier to separate out.

I'm not too sure about this. As it is not required to upload the android clients to the cloud deployment, I could imagine that people want to quickly iterate and test changes and try them out in a running cloud deployment. I simply don't think we should try to immediately connect, if there are now two ways to connect, but this is all code in the playground so developers will likely use a different interface to decide how to connect.

Not sure I understand what you mean by the second point. Do you mean how we decide whether to connect to a local or a cloud deployment?

Agree, this approach is more flexible.
For my second point I have a general feeling that we can improve the launch configuration and processing both in tooling and in Playground as a sample, but I don't have concrete suggestions as of the moment. Will try to think more about this and maybe come up with something more concrete.

gkassabli · 2019-01-08T21:16:53Z

workers/unity/Packages/com.improbable.gdk.mobile/Android/Utility/LaunchArguments.cs

@@ -8,6 +8,7 @@ public static class LaunchArguments
    {
        public static Dictionary<string, string> GetArguments()
        {
+#if !UNITY_EDITOR


Was this malfunctioning in Unity Editor?

Yes, as jamie and I were discussing in one of the previous threads in this PR: you can set your current platform to Android in the Editor, which will cause this code to be run. However using these java objects will only succeed in an actual android device/emulator. You end up with a nullpointer exception.

So either

we don‘t throw an exception

we don‘t run this code in the editor

We decided that not running it seems like the better option, because this code shouldn‘t fail on an actual android device

Would it be better to wrap this in Application.isEditor rather than compiling it out?

jared-improbable · 2019-01-09T08:02:48Z

workers/unity/Packages/com.improbable.gdk.core/Exceptions/AuthenticationFailedException.cs

+namespace Improbable.Gdk.Core
+{
+    /// <summary>
+    ///     Represents an error that occurs when the local state of a SpatialOS entity is not valid.


This doc doesn't seem to reflect the exception name

jared-improbable · 2019-01-09T08:05:20Z

workers/unity/Packages/com.improbable.gdk.mobile/Android/Utility/LaunchArguments.cs

@@ -8,6 +8,7 @@ public static class LaunchArguments
    {
        public static Dictionary<string, string> GetArguments()
        {
+#if !UNITY_EDITOR


Would it be better to wrap this in Application.isEditor rather than compiling it out?

jamiebrynes7

Mostly LGTM - one small thing I'm not sure about.

jamiebrynes7 · 2019-01-09T10:41:53Z

workers/unity/Packages/com.improbable.gdk.core/Worker/WorkerConnector.cs

+                throw new AuthenticationFailedException("Did not receive a player identity token.");
+            }
+
+            if (!string.IsNullOrEmpty(result.Value.Error))


I think you should only check for null as the underlying C API returns a null pointer if there is no error. Not sure how this gets transformed into the C# struct. cc @davedissian

Checked with David, we can actually just use the status code

jamiebrynes7 · 2019-01-09T10:42:48Z

workers/unity/Packages/com.improbable.gdk.core/Worker/WorkerConnector.cs

+        }
+
+        /// <summary>
+        ///     Retrieves the login tokens of all active deployments that the player 


Nit: Retrieves the login tokens for all active deployments...

improbable-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 40-149 lines, ignoring generated files. labels Dec 20, 2018

jared-improbable reviewed Dec 20, 2018

View reviewed changes

jessicafalk added 2 commits January 7, 2019 09:13

adding anonymous auth

f70e6e8

making it configurable

c468489

jessicafalk force-pushed the feature/anon-auth branch from f91e915 to c468489 Compare January 7, 2019 15:08

improbable-prow-robot added size/XXL Denotes a PR that changes 600+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 40-149 lines, ignoring generated files. labels Jan 7, 2019

jessicafalk changed the title ~~[WIP] anonymous auth for mobile~~ anonymous auth for mobile Jan 7, 2019

improbable-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 7, 2019

jessicafalk requested review from jamiebrynes7, gkassabli and zeroZshadow January 7, 2019 15:33

gkassabli reviewed Jan 7, 2019

View reviewed changes

jamiebrynes7 suggested changes Jan 7, 2019

View reviewed changes

jessicafalk added 3 commits January 8, 2019 10:57

PR feedback

829728c

added changelog

04f628b

some cleaning up

1a9d496

jamiebrynes7 reviewed Jan 8, 2019

View reviewed changes

workers/unity/Packages/com.improbable.gdk.core/Exceptions/AuthenticationFailedException.cs Outdated Show resolved Hide resolved

workers/unity/Packages/com.improbable.gdk.core/Worker/Worker.cs Outdated Show resolved Hide resolved

jessicafalk and others added 3 commits January 8, 2019 11:55

remove whitespace

f04692a

changed from internal to public

d1e290e

Merge branch 'master' into feature/anon-auth

d17d10a

jamiebrynes7 added A: mobile Area: Mobile integration T: feature Type: This is a new feature labels Jan 8, 2019

jessicafalk added 3 commits January 8, 2019 17:29

adding exit code check

c921543

Merge branch 'feature/anon-auth' of github.com:spatialos/gdk-for-unit…

7ff5ebe

…y into feature/anon-auth

stop trying to connect to local deployment automatically

5486e08

gkassabli reviewed Jan 8, 2019

View reviewed changes

gkassabli approved these changes Jan 9, 2019

View reviewed changes

jared-improbable reviewed Jan 9, 2019

View reviewed changes

added correct docs and changed how to check for the editor

7fed548

jamiebrynes7 approved these changes Jan 9, 2019

View reviewed changes

more PR feedback

5d0930d

jessicafalk merged commit 5d4775b into master Jan 9, 2019

jamiebrynes7 deleted the feature/anon-auth branch January 9, 2019 18:43


		public async void TryConnect()
		public async void TryConnect(ConnectionService connectionService)

anonymous auth for mobile #660

anonymous auth for mobile #660

Conversation

jessicafalk commented Dec 20, 2018 • edited Loading

Description

Tests

Documentation

Primary reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkassabli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiebrynes7 commented Jan 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiebrynes7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicafalk commented Dec 20, 2018 •

edited

Loading