[AGDROID-771] Batch security metrics and update json schema #126

paolobueno · 2018-03-02T16:23:26Z

Motivation

JIRA: https://issues.jboss.org/browse/AGDROID-771

Description

Make both Sync and Async executors send a single metrics request upon finishing execution.
Update data being sent to match the server-side implementation at aerogear-attic/aerogear-app-metrics#33

Progress

Add support for multiple results
Update call from sync executor
Update call from async executor
Update unit tests
Add type field
Change syncing mechanism to syncroninzed keyword
Make metrics interface generic

paolobueno · 2018-03-02T16:24:51Z

auth/build.gradle

-        unitTests.includeAndroidResources = true
+        unitTests {
+            includeAndroidResources = true
+            returnDefaultValues = true


I did this because of http://tools.android.com/tech-docs/unit-testing-support#TOC-Method-...-not-mocked.- errors.

Not sure if it's okay but seemed easier than mocking and DI-ing JSONObject and Looper.

paolobueno · 2018-03-02T16:36:55Z

auth/src/main/java/org/aerogear/mobile/security/AsyncSecurityCheckExecutor.java

+        final ArrayList<SecurityCheckResult> results = new ArrayList<>(howMany);
+        for (int i = 0; i < howMany; i++) {
+            try {
+                SecurityCheckResult result = cs.take().get();


This seems to be the recommended way to consume these but I'm not sure if it's very solid.

@secondsun @danielpassos Is there a risk these could get mixed up with other tasks in the same ExecutorService?
It seems that by default it's the AppExecutors#singleThreadService() singleton and this method is set to be run in the networkThread().

What do you mean by "mixed up"?

@secondsun maybe the race condition is only in my head.

IIUC the executorService is a queue so I thought that maybe if somewhere else in the main thread other kinds of jobs are queued into the singleThreadService the cs.take() call could end up picking a result that's meant for another async operation.

Ahhh right. So 1) The network thread is single threaded. That isn't guaranteed, but it is right now. 2) If the order is important there are options for enforcing the order of the jobs, but I will have to refresh myself. (Or we add a epic for weaving RxJava into the SDK implementation)

@paolobueno With the suggested approach, this problem does not exists (see my comment)

paolobueno · 2018-03-02T16:39:37Z

core/src/main/java/org/aerogear/mobile/core/metrics/Metrics.java

 public interface Metrics {

    String identifier();

-    Map<String, String> data();
+    Object data();


Had to do this to support JSONArray. JSONArray and JSONObject don't share any ancestors.

The goal is to be able to have an entry with the shape:

{ {{identifier}}: [data] }

Both implementations we provide use JSONObject, so why not a JSONObject? In the future if we need this then we can raise the interface's type without breaking compatibility.

Alternatively we can define the the Metrics data as a generic type.

We want to reach the following shape from aerogear-attic/aerogear-app-metrics#33:

"security": [ { "type": "org.aerogear.mobile.security.checks.DeveloperModeCheck", "passed": true }, { "type": "org.aerogear.mobile.security.checks.EmulatorCheck", "passed": false }, { "type": "org.aerogear.mobile.security.checks.DebuggerCheck", "passed": false }, { "type": "org.aerogear.mobile.security.checks.RootedCheck", "passed": false }, { "type": "org.aerogear.mobile.security.checks.ScreenLockCheck", "passed": false } ]

So I changed SecurityCheckResultMetric to be {identifier: "security", data: JSONArray()}.
I guess we could somehow change NetworkMetricsPublisher#publish instead.

Do you see any benefit of having Metrics<T> without a type constraint over Object?

I didn't see much point doing that because even https://developer.android.com/reference/org/json/JSONObject.html#put(java.lang.String,java.lang.Object) receives Object over having overloads for JSONObject|Array

The best thing that a generic type parameter gets us is a little bit of developer ease at code writing time. Depending on how much people will put in their Metrics implementations it may or may not be beneficial.

What the generic type does is make explicit the data type that is being wrapped by the metrics class. Functionality wise it won't behave differently than the current proposal, but it does make the down typing you can do on the data method more explicit.

secondsun · 2018-03-02T17:09:20Z

I'm interested in seeing how this works. Mind tossing in some steps for how to test it using openshift and the mobile features?

paolobueno · 2018-03-02T17:23:08Z

@secondsun thanks for the review, I only paid attention to the unit tests so far, going to sync up with @darahayes because verification would require aerogear-attic/aerogear-app-metrics#33 too.

I guess it'd actually be the goal of https://issues.jboss.org/browse/AEROGEAR-2218 too.

darahayes · 2018-03-05T10:37:32Z

@paolobueno can you please revisit the server side PR here: aerogear-attic/aerogear-app-metrics#33

The server now expects the security checks to have a name field for displaying human readable labels in graphs/charts.

The format should now look like this:

    "security": [
      {
        "type": "org.aerogear.mobile.security.checks.DeveloperModeCheck",
        "name": "Developer Mode Check",
        "passed": true
      },
     {
        "type": "org.aerogear.mobile.security.checks.EmulatorCheck",
        "name": "Emulator Check",
        "passed": false
      },
      {
        "type": "org.aerogear.mobile.security.checks.DebuggerCheck",
        "name": "Debugger Check",
        "passed": false
      },
      {
        "type": "org.aerogear.mobile.security.checks.RootedCheck",
        "name": "Rooted Check",
        "passed": false
      },
      {
        "type": "org.aerogear.mobile.security.checks.ScreenLockCheck",
        "name": "Screen Lock Check",
        "passed": false
      }
    ]
  }

ziccardi · 2018-03-05T09:19:49Z

auth/src/main/java/org/aerogear/mobile/auth/AuthService.java

@@ -40,7 +40,8 @@
    private OIDCAuthenticatorImpl oidcAuthenticatorImpl;

    private Context appContext;
-    private Logger logger;
+    private final Logger LOG = MobileCore.getLogger();


Make this static

ziccardi · 2018-03-05T11:06:44Z

auth/src/main/java/org/aerogear/mobile/security/AsyncSecurityCheckExecutor.java

-                if (metricsService != null) {
-                    metricsService.publish(new SecurityCheckResultMetric(result));
-                }
+        final Collection<SecurityCheck> checks = getChecks();


I think it would be easier like this:

public Map<String, Future<SecurityCheckResult>> execute() { final MetricsService metricsService = getMetricsService(); final Map<String, Future<SecurityCheckResult>> res = new HashMap<>(); final List<SecurityCheckResultMetric> metrics = new ArrayList<>(); final Collection<SecurityCheck> checks = getChecks(); for (final SecurityCheck check : checks) { res.put(check.getName(), (executorService.submit(() -> { final SecurityCheckResult result = check.test(getContext()); if (metricsService != null) { synchronized (metrics) { metrics.add(new SecurityCheckResultMetric(result)); if (metrics.size() == checks.size()) { metricsService.publish(metrics.toArray(new SecurityCheckResultMetric[metrics.size()])); } } } return result; }))); } return res; }

I see, wasn't familiar with synchronized methods and blocks.

I was, however, thinking about ways that users could do this kind of queuing of actions themselves. Both of these approaches are only possible because we're in control of the Callables or aware of the ExecutorService.

I guess we can postpone these improvements for Rx 🙏

@paolobueno TBH I think we shouldn't change the executor to batch the metrics, but batching should be done by the metric service (see my comment at the end of the PR)

josemigallas

I don't quite understand why AsyncSecurityCheckExecutor is handled like that. Wouldn't it be possible to do something like wrapping SyncSecurityCheckExecutor in a Future?

josemigallas · 2018-03-05T09:33:41Z

auth/src/main/java/org/aerogear/mobile/auth/AuthService.java

@@ -40,7 +40,8 @@
    private OIDCAuthenticatorImpl oidcAuthenticatorImpl;

    private Context appContext;
-    private Logger logger;
+    private final Logger LOG = MobileCore.getLogger();


LOGGER would be more proper.

I suggest private finals are declared before standard fields and after public finals.

josemigallas · 2018-03-05T09:44:55Z

auth/src/main/java/org/aerogear/mobile/auth/credentials/OIDCCredentials.java

@@ -62,7 +61,7 @@ public String getRefreshToken() {

    /**
     * Verify the token and its claims against the given Keycloak configuration
-     * 
+     *


This is why we need to fix the linter/format issues once for all

josemigallas · 2018-03-05T09:52:41Z

auth/src/main/java/org/aerogear/mobile/security/AsyncSecurityCheckExecutor.java

+        final int checkCount = checks.size();
+        for (final SecurityCheck check : checks) {
+            final Future<SecurityCheckResult> future = executorService.submit(() -> {
+                final SecurityCheckResult result = check.test(getContext());


This can be in one line:
return check.test(getContext());
or even:
executorService.submit(() -> check.test(getContext()));

josemigallas · 2018-03-05T09:53:10Z

auth/src/main/java/org/aerogear/mobile/security/AsyncSecurityCheckExecutor.java

-                    metricsService.publish(new SecurityCheckResultMetric(result));
-                }
+        final Collection<SecurityCheck> checks = getChecks();
+        final int checkCount = checks.size();


I don't think this constant is needed. In any case I would declare it after the loop, for readability.

josemigallas · 2018-03-05T09:53:40Z

auth/src/main/java/org/aerogear/mobile/security/AsyncSecurityCheckExecutor.java

+            res.put(check.getName(), future);
+        }
+
+        if(metricsService != null) {


This is missing a space if (..

We definitely need to fix the linter/format issue.

I think the constant metricsService is unnecessary or in any case I would declare it right before using it. They it is, it looks like a field. Moreover,publishMetricsAsync is using the getter directly.

josemigallas · 2018-03-05T10:39:49Z

auth/src/main/java/org/aerogear/mobile/security/SyncSecurityCheckExecutor.java

-
-        if (metricsService != null) {
-            metricsService.publish(new SecurityCheckResultMetric(nonNull(result, "result")));
+    private void publishResultMetrics(final @NonNull Map<String, SecurityCheckResult> results) {


This is a small detail but publishResultMetrics shouldn't check if metricsService is null. That should be checked before calling this method. Otherwise the method should be called something like publishResultMetricsIfPossible. In AsyncSecurityCheckExecutor it is checked this way for instance.

josemigallas · 2018-03-05T10:45:22Z

auth/src/main/java/org/aerogear/mobile/security/metrics/SecurityCheckResultMetric.java

+    private final Logger LOG = MobileCore.getLogger();
+    private final String TAG = "SecurityCheckResultMetric";
+
+    public static final String IDENTIFIER = "security";


I suggest declaring public static finals on top of anything, for better visibility.

josemigallas · 2018-03-05T10:49:21Z

auth/src/main/java/org/aerogear/mobile/security/metrics/SecurityCheckResultMetric.java

-    public SecurityCheckResultMetric(@NonNull final SecurityCheckResult result) {
-        this.identifier = nonNull(result, "result").getName();
-        this.data = getDataFromResult(result);
+    public SecurityCheckResultMetric(@NonNull final SecurityCheckResult... results) {


Do we need this constructor? It's used only in a test.

josemigallas · 2018-03-05T11:04:56Z

auth/src/main/java/org/aerogear/mobile/security/metrics/SecurityCheckResultMetric.java

@@ -48,19 +66,32 @@ public String identifier() {
     * the value is <code>true</code> if the check result passed
     */
    @Override
-    public Map<String, String> data() {
-        return Collections.unmodifiableMap(data);
+    public JSONArray data() {


Please update doc comments.

josemigallas · 2018-03-05T11:15:21Z

auth/src/main/java/org/aerogear/mobile/security/metrics/SecurityCheckResultMetric.java

-    public Map<String, String> data() {
-        return Collections.unmodifiableMap(data);
+    public JSONArray data() {
+        // TODO: consider returning a deep clone


I would not create the JSONArray during instantiation but rather store the results as class field and return a new JSONArray each time:

public SecurityCheckResultMetric(@NonNull final Iterable<SecurityCheckResult> results) { this.results = results; } public SecurityCheckResultMetric(@NonNull final SecurityCheckResult... results) { this.results = Arrays.asList(results); } public JSONArray data() { return getDataFromResult(this.results); }

Also I'd add a test to ensure data cannot be changed if this would be a problem.

ziccardi · 2018-03-05T13:55:46Z

@josemigallas as you can see here, the SyncSecurityCheckExecutor executes the checks one after the other.
If you would wrap it in a Future:

All the checks would be executed in just one thread
The code will be less customizable

While, on the other hand, the code to be written would be almost the same.

ziccardi · 2018-03-05T14:09:34Z

auth/src/main/java/org/aerogear/mobile/security/AsyncSecurityCheckExecutor.java

        }

        return res;
    }
+
+    private void publishMetricsAsync(final int howMany) {


With the suggested approach, this method is useless

paolobueno · 2018-03-05T15:17:25Z

I think having a single sync Executor and running it in a background thread could be enough. Paralellizing all the self-defence checks seems like a premature optimization, though the code is mostly in place.

Do you guys think there'll be any long-running self-defence check that would warrant being run in parallel?

Wasn't planning on doing this change as part of this PR though.

ziccardi · 2018-03-05T16:10:39Z

@paolobueno Not sure I get it. The AsyncSecurityCheckExecutor is able to execute checks asynchronously using a passed in ExecutorService, so if a multithread ExecutorService is passed, checks are automatically executed in parallel.

I thought this PR was just about batching the metrics: even about that, I think we are making the code unnecessarily complex.
If we want the metrics to be batched, that should be done by the metric service object.
If we want the user to be able to control the batch size (not sure we want that), then instead of passing a metric service object to the executor, we should pass a metric service publisher object which can be configured with the batch size.

SecurityCheckExecutor.newAsyncCheckExecutor(context)
...
...
.withMetricsPublisher(new MetricPublisher(10))
.build().execute();

Then the publisher could (in a background thread):

publish the metrics whenever the list of received metrics gets bigger than X
publish the metrics every X seconds since the last time metrics have been published

@secondsun @wei-lee : WDYT?

paolobueno · 2018-03-05T17:16:05Z

Making the publisher smarter was mentioned in https://issues.jboss.org/browse/AGDROID-771?focusedCommentId=13540352&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13540352 too.

Agree it'd be nice to have but I'd like to leave this out of the scope of this PR if possible.

ziccardi · 2018-03-05T17:43:52Z

auth/src/main/java/org/aerogear/mobile/security/AsyncSecurityCheckExecutor.java

+
+        int checkCount = getChecks().size();
+        if (results == null) {
+            results = new ArrayList<>(checkCount);


This way if the called will call 2 times the execute method, metrics will be messed up and some metric will be lost.
With the proposed approach, we would not have this issue.

I thought the method being synchronized would prevent this.

Nope. The result array is a member variable. If I call the execute two times, I will add the results twice in the same array

Ah, I see what you mean, the executor interface doesn't prevent it being executed multiple times. 👍

danielpassos · 2018-03-05T22:48:55Z

@paolobueno If I do execute from securityService.getCheckExecutor() the metricsService is always be null on publishResultMetrics. So how I suppose to use that?

Here is an example what I did:

securityService.getCheckExecutor()
    .addCheck(SecurityCheckType.ALLOW_BACKUP_ENABLED)
    .addCheck(SecurityCheckType.IS_EMULATOR)
    .addCheck(SecurityCheckType.IS_DEBUGGER)
    .addCheck(SecurityCheckType.IS_DEVELOPER_MODE)
    .addCheck(SecurityCheckType.IS_ROOTED)
    .addCheck(SecurityCheckType.SCREEN_LOCK_ENABLED)
    .addCheck(SecurityCheckType.HAS_ENCRYPTION_ENABLED)
    .execute();

Added event listener

ziccardi · 2018-03-06T09:50:05Z

auth/src/main/java/org/aerogear/mobile/security/AsyncSecurityCheckExecutor.java

        }

        return res;
    }
+
+    private synchronized void tallyResultsAndPublish(SecurityCheckResult result, List<SecurityCheckResult> results) {


@paolobueno This way it would work, however synchronising the whole method, you synchronize parts of the code that doesn't needs to be synchronized (thus slowing the execution).

I have created a PR to propose a different implementation for the batching. Would you mind giving that a look?
#129

ziccardi · 2018-03-06T10:04:37Z

@danielpassos That's true. IMHO I think that the getCheckExecutor, getAsyncCheckExecutor, checkAndSendMetric(s) methods are confusing and makes the code less dynamic/maintainable.

To do what you want we should add a setMetricService method to the executor

However addCheck and eventually the setMetricService will create RACE conditions if the same executor instance is used in two different threads.

That does not happen if we use directly the builders since they were designed to produce immutable objects. Using the builders, your code would be:

SecurityCheckExecutor
    .newSyncExecutor(getContext())  // or newAsyncExecutor(getContext()) if you need an async
    .withSecurityCheck(SecurityCheckType.ALLOW_BACKUP_ENABLED)
    .withSecurityCheck(SecurityCheckType.IS_EMULATOR)
    .withSecurityCheck(SecurityCheckType.IS_DEBUGGER)
    .withSecurityCheck(SecurityCheckType.IS_DEVELOPER_MODE)
    .withSecurityCheck(SecurityCheckType.IS_ROOTED)
    .withSecurityCheck(SecurityCheckType.SCREEN_LOCK_ENABLED)
    .withSecurityCheck(SecurityCheckType.HAS_ENCRYPTION_ENABLED)
    .withMetricService(metricService)
    .build()
    .execute();

wei-lee · 2018-03-06T10:31:51Z

@paolobueno can you take a look at PR #129. I think it is a much simpler and cleaner way to implement this function.

…from the end user

paolobueno · 2018-03-06T13:25:56Z

@ziccardi rebased this on #129 and removed the java.util.concurrent-based approach, only change I needed on top of it was ed523c6 in order to put the metrics inside a "security": key as per the schema from the metrics server.

ziccardi

Minor comments, then LGTM

ziccardi · 2018-03-06T13:31:42Z

auth/src/main/java/org/aerogear/mobile/security/SecurityCheckExecutorListener.java

+    /**
+     * Called when all submitted checks has been executed.
+     */
+    void onFinished();


as suggested by @matzew in the other PR, rename this to onComplete

paolobueno requested review from danielpassos, wei-lee, secondsun and darahayes March 2, 2018 16:23

paolobueno commented Mar 2, 2018

View reviewed changes

paolobueno force-pushed the AGDROID-771-batch-security-metrics branch from 71e966e to 548d938 Compare March 2, 2018 16:41

paolobueno requested a review from josemigallas March 2, 2018 16:52

wei-lee requested a review from ziccardi March 2, 2018 17:21

paolobueno mentioned this pull request Mar 2, 2018

feat: initial parsing of security metrics aerogear-attic/aerogear-app-metrics#33

Merged

paolobueno changed the title ~~[AGDROID-771] Batch security metrics and update json schema~~ WIP [AGDROID-771] Batch security metrics and update json schema Mar 4, 2018

ziccardi reviewed Mar 5, 2018

View reviewed changes

josemigallas reviewed Mar 5, 2018

View reviewed changes

ziccardi reviewed Mar 5, 2018

View reviewed changes

ziccardi suggested changes Mar 5, 2018

View reviewed changes

Added metric publishing batching

a9e4f42

Added event listener

ziccardi suggested changes Mar 6, 2018

View reviewed changes

ziccardi added 2 commits March 6, 2018 12:18

Removed double for loop

63af571

Removed if, removed double call to getChecks

109a208

paolobueno mentioned this pull request Mar 6, 2018

Added metric publishing batching #129

Merged

ziccardi and others added 11 commits March 6, 2018 14:18

Made the new method accepting a list of listener private, to hide it …

322edbf

…from the end user

Change networkExecutor type to ExecutorService

5ce5e77

Update metric api to support multiple results

ea8971c

Change Metrics interface to use JSONObject over Map<String, String>

0e9ba31

Improve logging calls

94d7b09

Fallback to Object on Metrics interface in order to accept JSONArray

98fc804

Fix tests

5b5357c

Use final

7d6ef9b

Fix logging for Metrics

6f85e4f

Make Metrics generic

8a7df80

Build only a single result metric

ed523c6

paolobueno force-pushed the AGDROID-771-batch-security-metrics branch from 7e84d68 to ed523c6 Compare March 6, 2018 13:22

ziccardi reviewed Mar 6, 2018

View reviewed changes

Rename listener interface method to onComplete

73b17fb

ziccardi approved these changes Mar 6, 2018

View reviewed changes

paolobueno changed the title ~~WIP [AGDROID-771] Batch security metrics and update json schema~~ [AGDROID-771] Batch security metrics and update json schema Mar 6, 2018

paolobueno merged commit c7df5c6 into aerogear:master Mar 6, 2018

paolobueno deleted the AGDROID-771-batch-security-metrics branch March 6, 2018 13:45

[AGDROID-771] Batch security metrics and update json schema #126

[AGDROID-771] Batch security metrics and update json schema #126

Conversation

paolobueno commented Mar 2, 2018 • edited

Motivation

Description

Progress

paolobueno Mar 2, 2018 • edited

Choose a reason for hiding this comment

paolobueno Mar 2, 2018 • edited

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

ziccardi Mar 5, 2018 • edited

Choose a reason for hiding this comment

paolobueno Mar 2, 2018 • edited

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

secondsun commented Mar 2, 2018

paolobueno commented Mar 2, 2018 • edited

darahayes commented Mar 5, 2018

Choose a reason for hiding this comment

ziccardi Mar 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josemigallas 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

ziccardi commented Mar 5, 2018 • edited

ziccardi Mar 5, 2018 • edited

Choose a reason for hiding this comment

paolobueno commented Mar 5, 2018

ziccardi commented Mar 5, 2018 • edited

paolobueno commented Mar 5, 2018

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

danielpassos commented Mar 5, 2018

Choose a reason for hiding this comment

ziccardi commented Mar 6, 2018

wei-lee commented Mar 6, 2018

paolobueno commented Mar 6, 2018

ziccardi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paolobueno commented Mar 2, 2018 •

edited

paolobueno Mar 2, 2018 •

edited

paolobueno Mar 2, 2018 •

edited

ziccardi Mar 5, 2018 •

edited

paolobueno Mar 2, 2018 •

edited

paolobueno commented Mar 2, 2018 •

edited

ziccardi Mar 5, 2018 •

edited

ziccardi commented Mar 5, 2018 •

edited

ziccardi Mar 5, 2018 •

edited

ziccardi commented Mar 5, 2018 •

edited