-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
Tech5 Integration - Enrollment updates #2701
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
public class BiometricUtils { | ||
|
||
public enum BiometricIdentifier { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
potentially worth to put into a separate file of it's own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
public class BiometricUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code formatting is off in the class.
// Base64 encoded String | ||
Map<Integer, String> templatesBase64Encoded = new HashMap<>(templates.size()); | ||
for (Map.Entry<BiometricUtils.BiometricIdentifier, byte[]> template : templates.entrySet()) { | ||
templatesBase64Encoded.put(template.getKey().ordinal(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason we are double encoding templates, here and then later again at the end of this method ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because JSON
is used to represent the Map object, and considering that the values are byte arrays, the first encoding avoids representing them in JSON
, which would increment the size due to the comas used to split the elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am struggling to understand how will this gets used in the form. With current design, Seems like we are going to store into form the whole template
string instead of storing the individual templates in individula field like Simprint. Any reason we are deviating away from Simprints pattern ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubham1g5 the principle here is to use CC just to store the biometric templates and in the most simple way, so in a case property. any biometric operations will be preceded by the retrieval and restoration of the templates to their original format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant why not store individiual templates in separate fields in form like here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more to avoid having to handle different form questions and case properties, when there is no apparent advantage to doing that. From my understanding, we are not doing anything with the biometric templates in CC. But I think that for the sake of standartization we could adopt the same pattern, but I wonder if you see any other gains from this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we store it together and an app is capturing multiple templates, how will they separate this data out in future without writing a script ? I think storing them seprately makes any future data management on those templates much simpler for project teams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bee8e72b93a3b35d429a6ebc260b5df36b1bfa35
@damagatchi retest this please |
fun testRegistrationWithTemplates() { | ||
val formEntryActivity = ActivityLaunchUtils.launchFormEntry("m0-f1") | ||
|
||
var templates : HashMap<BiometricIdentifier, ByteArray> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to be duplicated here and then again in intendRegistrationWithTemplatesIntent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, in the principle of separation of concerns, but we can move it to a global var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just pass the templates as a method param instead of making it a global var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a better option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -20,6 +20,11 @@ Often you will need this generated guid to be passed back to CommCare so that it | |||
IdentityResponseBuilder.registrationResponse(guid) | |||
.finalizeResponse(activity) | |||
```` | |||
Alternatively, in case the biometric templates are to be stored in CommCare, use the following instead - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add info on how a third party should construct templates as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, I missed that one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c49a74e
@@ -0,0 +1,7 @@ | |||
package org.commcare.commcaresupportlibrary; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make sure that all biometric classes are in org.commcare.commcaresupportlibrary.identity
package.
@@ -0,0 +1,7 @@ | |||
package org.commcare.commcaresupportlibrary; | |||
|
|||
public enum BiometricIdentifier { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we wanna allow UNKNOWN
or INVALID
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Base64 encoded String | ||
Map<Integer, String> templatesBase64Encoded = new HashMap<>(templates.size()); | ||
for (Map.Entry<BiometricUtils.BiometricIdentifier, byte[]> template : templates.entrySet()) { | ||
templatesBase64Encoded.put(template.getKey().ordinal(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am struggling to understand how will this gets used in the form. With current design, Seems like we are going to store into form the whole template
string instead of storing the individual templates in individula field like Simprint. Any reason we are deviating away from Simprints pattern ?
* @param base64EncodedTemplates String containing Base64 encoded biometric templates | ||
* @return Map containing biometric templates | ||
*/ | ||
public static Map<BiometricIdentifier, byte[]> convertBase64StringTemplatesToMap( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where will this be used ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the biometric app, once the biometric templates are retrieved from the CC database, this will them be used to revert them into the original form.
@damagatchi retest this please |
@@ -212,4 +236,11 @@ public static boolean isRegistrationResponse(Intent intent) { | |||
public static boolean isVerificationResponse(Intent intent) { | |||
return intent.hasExtra(IdentityResponseBuilder.VERIFICATION); | |||
} | |||
|
|||
public static void main(String[] args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is main
useful here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoot, just residual code from some testing I was conducting, I could swear that I removed everything by somehow some was left behind. My bad.
if (registrationResult.getTemplates().isEmpty()) { | ||
result = guid; | ||
} else { | ||
result = Localization.get("intent.callout.biometrics.capture.result", | ||
new String[]{String.valueOf(numOfTemplatesStored), String.valueOf(numOfTemplates)}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we store this for Simprints, but we should be consistent in our implementation and can always store guid
in the node value. (Unless you see a strong reason to do otherwise)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning here is to provide some feedback to the user in terms of the number of biometrics that was successfully captured (and stored). It can be that we scan 4 fingers but the callout handler is only able to store 2 due to a misconfiguration in the app. But not a hard requirement, I'm happy to drop this and maybe wait for feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. That makes sense to me although I am worried about supporting 2 different return formats to the callout question. I am thinking about case exports where some fields might have guid and some fields this string which might be confusing to interpret for the data systems incorporated by a project.
That said I do like the idea of telling to user whether everything was saved correctly or not. How about we just say All data stored successfully
or Failed to store some data
to indicate failure vs success in both templates and non templates use cases here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String templatesInJson = new Gson().toJson(templatePairBase64Encoded); | ||
return Base64.encodeToString(templatesInJson.getBytes(), Base64.DEFAULT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why convert to Json before encoding instead of just doing Base64.encodeToString(template, Base64.DEFAULT)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the idea is to bundle the BiometricIdentifier
with the template
. It is redundant as we use the callout response key
to identify the type of biometric but I think it could be useful to have a second layer of identification. For instance, the app builder can make a poor choice of case property name and have difficulties later to know if the template is of a finger
or a face
. We could also use the position 0
of the array for this. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance, the app builder can make a poor choice of case property name and have difficulties later to know if the template is of a finger or a face
I don't think its a valid concern. App builders can name the properties of other fields poorly too (First Name to Last Name for ex) and we should not take into account such mistakes to make decisions at platform level. I would rather rgo for simplicity here and just store the template directly in the case property.
@@ -1,58 +1,91 @@ | |||
<h:html xmlns:h="http://www.w3.org/1999/xhtml" xmlns:orx="http://openrosa.org/jr/xforms" xmlns="http://www.w3.org/2002/xforms" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:jr="http://openrosa.org/javarosa" xmlns:vellum="http://commcarehq.org/xforms/vellum"> | |||
<h:head> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: diffs like this are very hard to review and should be avoided. Will you be able to highlight what's changed here with comments at the very least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is confusing, do we have a practice around this? I just made the modification in HQ, downloaded the app and replaced the files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only change the parts that changed. I generally edit the files by hand manually to add the new/modified parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I undid this and followed your advice - e81b21b
@@ -96,6 +99,25 @@ class IdentityCalloutTests { | |||
assertEquals(guidToConfidenceMap.elementAt(2), "★★★") | |||
} | |||
|
|||
@Test | |||
fun testRegistrationWithTemplates() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also verify that converting back base 64 encoded templates to original templates using the support lib method indeed matches with original templates as part of this test.
IdentityResponseBuilder.registrationResponse(guid, templates) | ||
.finalizeResponse(activity) | ||
```` | ||
* `templates` is a [`Map`](https://docs.oracle.com/javase/8/docs/api/java/util/Map.html) containing all the biometric templates and whose _keys_ are [`BiometricIdentifier`](BiometricIdentifier.java) elements and _values_ are the actual biometric templates in the form of a byte array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a sample code below this to give an example of how to construct templates using Biometric indentifiers.
c49a74e
to
1d1d439
Compare
public static Object getFormValue(String expr) throws XPathSyntaxException { | ||
FormDef formDef = FormEntryActivity.mFormController.getFormEntryController().getModel().getForm(); | ||
FormInstance instance = formDef.getMainInstance(); | ||
EvaluationContext ec = new EvaluationContext(instance); | ||
return ExprEvalUtils.xpathEval(ec, expr); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubham1g5 I have created this method to retrieve a value from a form question, but I wonder if there isn't a method to do this already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, one minor comment.
@@ -57,6 +57,8 @@ dependencies { | |||
implementation fileTree(dir: 'libs', include: ['*.jar']) | |||
implementation 'com.android.support:appcompat-v7:26.1.0' | |||
implementation 'com.android.support.constraint:constraint-layout:1.0.2' | |||
implementation 'com.google.code.gson:gson:2.9.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, it's not needed.
Summary
This PR is part of an ongoing initiative to integrate CommCare with Tech5 SDK. The goal of the current changes is to be able to capture biometric templates and send back to CommCare in a Base64 encoded String as part of a app callout question response.
Safety Assurance