Mc 1568 od

[gorandostanic]

Add operator discovery functionality to Connect ID Android SDK allowing it to be used as MobileConnect SDK.

More info: https://confluence.skunk-works.no/jira/browse/MC-1568

[atanasdb]

Could you make the WellKnownApi class available to ConnectId SDK profile as well, because we'll want to use the network_authentication_target_ips for the Force HE.

[gorandostanic]

According to the book I am reading right now (Effective Java, remember? :-) it is always wise not to allow alien code to be executed in your synchronised block/method. Having read the rationale behind this, I couldn't agree more :)

I need an implementation of OperatorDiscoveryConfig interface to hold the defensive copy of the data. Hence the Plain-bla-bla thing.

ExampleOperatorDiscoveryConfig cannot serve that purpose because 1. It is in the example project outside the SDK 2. it is hard-coded 3. It is NOT our recommendation about how to implement that interface, just an example to make the example app work.

Actually, I have written a disclaimer in README.md about it, which motivated me to make the defensive copy of the interface:

The example implementation found in the example app is just a simple example. Developers are encouraged to implement their own scheme to keep this information secret.

[atanasdb]

Yeah nice, but according to the same book: "Classes containing methods or constructors whose invocation indicates a transfer of control cannot defend themselves against malicious clients. Such classes are acceptable only when there is mutual trust between the class and its client or when damage to the class’s invariants would harm no one but the client."

And that's what you have here. You do not have immutable class, that expose mutable objects through getters, and also messing up with the OperatorConfig would harm no one, but the client itself.

Also having an immutable bean defined in the SDK, let's say OperatorDiscoveryConfig, amd instantiating it in the app, and passing it to the SDK, would have the same effect, and the code will be less. (Like defining a simple struct in the SDK and initializing it in the client, in C++ terms :) )

[gorandostanic]

Done! (applies to refactoring onAuthorize)

[gorandostanic]

Regarding defensive copy: I'd like to keep it, if you agree (do you?).

The amount of code added is not very big, but it still insulates our code from (potentially hard to debug) concurrency issues a developer might cause. In addition, it prevents him from returning different values at different times, which we may want.

[atanasdb]

But having a single immutable class (let's say implemented with immutables.org we use all over) will give you the same thing, no ?

[phyrex1an]

I agree with atanas. I don't get what the current OperatorDiscoveryConfig interface is supposed to accomplish, compared with making it a "struct".

[gorandostanic]

Scrapped the defensive copy thing, since you are all hostile to it :)

[phyrex1an]

I wasn't hostile to the defensive copy, given the OperatorDiscoveryConfig interface. I just think that the interface is useless and should be an immutable object, and then you wont need the defensive copy.

Can you explain why OperatorDiscoveryConfig is an interface?

[gorandostanic]

Can you explain why OperatorDiscoveryConfig is an interface?

1. Because it can be :)
2. To avoid any interfering in how developers should implement their own secret keeping schema
3. To "wash our hands" from the deficiency in OD we have talked about (and I believe we agreed upon), namely the need of storing secrets within the app. This is kinda documented in README.md, as a disclaimer.

[phyrex1an]

You don't need an interface to accomplish 2 and 3. You just move the "defensive copy" out to the caller, how they store their secrets is up to them. The only difference is that their storage schema doesn't become mixed into an interface provided by us.

[atanasdb]

You will need to declare your references read/written by != threads with volatile.

[gorandostanic]

I asked Mikael decide, he preferred builder :)

[atanasdb]

I think you should just say (and make sure in callbacks) that all sdk methods are to be called from the UI thread. That programming model seems saner than trying to mark the right fields volatile everywhere.

The reasoning behind the volatile reference is that, they are written in the UI thread, but in the next PR (forced_he) they'll be consumed in the WebView and thus in a thread launched by it.

[gorandostanic]

What happens if the sdk is de-initialized during a mobile connect authentication for example?

Not much. Deinitialize() just flags the SDK to perform initialisation anew upon the next authenticate(). The current authentication (if any, since it is rather difficult to conceive one at this point of time) will be allowed to be performed with the old config.

That programming model seems saner than trying to mark the right fields volatile everywhere.

Atanas reacted to this remark already.

In many places (especially associated with initialize) you ignore the fact that some earlier method is not finished just because it returns, eg you have a callback but you try to go on as if the callback ran synchronously.

I think this is not wholly true. I believe I have made sure that the actual execution is always resumed from within the callback, it does not matter that it seemingly continues just like that. For example, initialize() is called only when a (sane) operatorDiscoveryResult is secured. If not, initialize() will never be called, and the activity will die. Anyway, I'd appreciate if you could be more specific here, I find Android development a bit weird, maybe I'm missing something substantial :)

[gorandostanic]

Now I’ve fixed the old unit tests and added some more for MC :-)

I think the things are more or less in place now... apart from the following:

1. The expected/actual/issuer/audience thing. I’ll do the fix it as soon as the related bug in comoyoauth has been fixed.

2. The translations (i.e. two untranslated strings used exclusively in Mobile Connect). We (i.e. the MC gang :) have discussed this issue recently and have come to a conclusion that we'd rather have this PR approved without translations this time, as being a very new thing. In addition, translations may take some time before they are ready to pick.

[atanasdb]

You have the "hack" for the staging environment of Telenor for the authorize host, which is fine, but you do not have the same behaviour for the .well-known uri construction which leads to problems with the forced he. Could you fix that?

[gorandostanic]

Sorry for that, I just happened to forget it, maybe because it shouldn't be used for MC, since the environment is decided upon client registration. I'll fix it as soon as I am on the plane :)

G.

On 24 May 2017 11:26, "Atanas Bakalov" notifications@github.com wrote:

You have the "hack" for the staging environment of Telenor for the authorize host, which is fine, but you do not have the same behaviour for the .well-known uri construction which leads to problems with the forced he. Could you fix that?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/telenordigital/connect-android-sdk/pull/50#issuecomment-303668939, or mute the thread https://github.com/notifications/unsubscribe-auth/AZwY4iZLw-wOz5YF9BkhOqdu3WZEPfogks5r8_e3gaJpZM4NNxXs .

[atanasdb]

Yes, best if you don't go over that url rewrite for MC at all.

[gorandostanic]

Connect ID: tested manually in accord with the test scenarios on:

SA Galaxy Trend 2 (SM-G313HN, Android 4.4.2) Yotaphone 2 (YD201, Android 5.0) SA Galaxy S5 (SM-GF00F, Android 6.0.1)

Wasn't quiet sure what 6. meant (Denying app permissions after authentication -> close sign in flow :-), so I skipped that one.

Regarding Mobile Connect: could not test in Belgrade fully because of a roaming issue (mcc/mnc), but performed the tests applicable to the current state of Mobile Connect SDK with some cheating.