-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
ConnectID Initial Implementation #2660
Conversation
Getting ready for network calls.
Initial registration workflow working. Implemented basic photos page and SMS verification.
Basic password manager implementation after ConnectID login.
@OrangeAndGreen Is it possible to add screenshots for different screens this PR adds in the description in order to get more context around the functionality here. Thanks! |
Implemented local encrypted DB for data related to Connect.
Some cleanup after achieving POC functionality. Using secure storage for password manager functionality.
Several UI improvements, rewording.
A couple small bug fixes. Preparing for OIDC calls.
Moved ConnectID activities into a sub-namespace called connect.
Showing recovery phone number in UI when sending alt OTP.
cross-request: dimagi/commcare-core#1290 |
Now encrypting DB passphrase using a Cipher, and storing the encrypted passphrase in the global DB. Added table to global DB for storing ConnectKeyRecords (changed DB version to 6). Min SDK Version moved back to 16 now that we aren't using newer helper classes (EncryptedFile, MasterKeys).
The various workflows are detailed via screenshots here. |
Not allowing password option when prompting for biometric unlock during registration. Fixed bug where ConnectID button on login page would appear after logging out of an app even if ConnectID user was not configured. Disabled auto-login to app after unlocking ConnectID.
…ing a broken test).
Checking if ConnectID is unlocked before retrieving SSO token. No null check on passphrase when retrieving from global DB.
…till use RSA after upgrading to API >=31).
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.
Very minor comments, looks quite close to merge. Can we resolve conflicts as well on this.
@@ -69,18 +69,29 @@ private static KeyStore getKeystore() throws KeyStoreException, CertificateExcep | |||
return keystoreSingleton; | |||
} | |||
|
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.
couple nits on this class-
- The Javadoc on this class seems misleading with the new changes in this class, can we modify it in accordance with the new functionality in this class.
- can we lint/format the code with CC style 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.
Done
return keyGenerator.generateKey(); | ||
} | ||
else { | ||
KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA", KEYSTORE_NAME); |
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.
user upgrades from API <31 to >=31.
should be when upgrading across API 23 right ?
A caveat of this solution is that we'll continue using RSA encryption after the user upgrades their OS
I am fine with this for now. I anticipate very few upgrades at this time for api 23 practically and just ensuring user is not locked from their data should be good enough handling for this case.
String transformation; | ||
if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.M) { | ||
transformation = String.format("%s/%s/%s", ALGORITHM, BLOCK_MODE, PADDING); | ||
if (forceRSA || android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.M) { |
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.
The android version check doesn't seem to be meaningful now to me. We can probably just decide the transformation based on forceRSA
?
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.
Yup, you're right, forceRSA will always be true for API<M. But we get a compile warning about using the ALGORITHM, BLOCK_MODE, and PADDING constants if I remove that check, so I left it there to keep the compiler happy. Would it be better to remove it and ignore/suppress the warning?
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.
Would it be better to remove it and ignore/suppress the warning?
Yeah I would support that since we are already doing this check outside to determine forceRSA
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.
Done, I removed the extra API check and suppressed the warning.
isBusy = false; | ||
dismissProgressDialog(context); |
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 abstract this into another internal method something like - onFinishProcessing()
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.
Done
Improved EncryptionUtils documentation, fixed a couple lint warnings, and reformatted code. Abstracted onFinishProcessing function in ConnectIDNetworkHelper.
…droid into dv/connect_id
@OrangeAndGreen the changes looks good though the build seems to be failing on Jenkins. |
…sed runtime crash).
…nto dv/connect_id
…xposed via CommCareApplication. Created MockEncryptionKeyProvider to allow encryption without Keystore in tests.
@@ -251,6 +253,11 @@ public void onCreate() { | |||
GraphUtil.setLabelCharacterLimit(getResources().getInteger(R.integer.graph_label_char_limit)); | |||
|
|||
FirebaseMessagingUtil.verifyToken(); | |||
|
|||
if(getEncryptionKeyProvider() == null) { |
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.
Think we can remove the null check here. As a best practice, child classed should be expected to call super.OnCreate()
as the first thing in the override method. They should be able to call setEncryptionKeyProvider
after super.OnCreate()
and override the defaults.
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.
Done
…de later). Some small code cleanup.
Auto-reformatted code in several files. Renamed several variables so acronyms use camel-case. Broke some longer lines into multiple.
Summary
Initial development of ConnectID functionality, including registration workflow.
Feature Flag
CommCare Connect
Product Description
When enabled, adds UI methods to LoginController for user to sign in to ConnectID.
First-time user can create an account with the Connect server.
Safety Assurance
Automated test coverage
No tests.
Safety story
This code is disabled by a hard-coded boolean at the top of the ConnectIDManager class, and when disabled results in no changes to existing functionality.
cross-request: dimagi/commcare-core#1318