Skip to content
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

fix: optimize getCipherStorageForCurrentAPILevel method #457

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Vito96
Copy link

@Vito96 Vito96 commented Apr 27, 2021

I reordered the execution of the instructions to optimize the getCipherStorageForCurrentAPILevel method.

In the case of isBiometry = false it is useless to execute the getCapabilityLevel method which can be calculated in case the RSA private key is missing.

I realized this because not using the warm up, the first de-encryption was still very slow due to this particular.

Furthermore, this re-order does not cause problems for the warm up because it is valued there with isBiometry = true.

@vonovak
Copy link
Collaborator

vonovak commented Apr 30, 2021

hello and thank you for the PR! Can you please take a look at the failing test and make it pass? Thank you! 💯

@Vito96
Copy link
Author

Vito96 commented May 21, 2021

   Test testGetSecurityLevel_NoBiometry_NoSecuredHardware_api28 FAILED

   java.lang.AssertionError:
   Expected: not null
        but: was null
       at com.oblador.keychain.KeychainModuleTests.testGetSecurityLevel_NoBiometry_NoSecuredHardware_api28 (KeychainModuleTests.java:433)
       

Why is this RSA test required?

@vonovak
Copy link
Collaborator

vonovak commented May 21, 2021

Why is this RSA test required?

Hello, I'm on mobile so I don't see the details of the test. Maybe it's not neccessary any more, maybe the test should be modified - my point is, we cannot merge a PR if it makes tests fail.

We need to have a clear understanding of whether your change introduced a bug or whether the failing test should be modified.

If you have time to claeify that, perfect. Otherwise I'll try to get to it at some point.

@oblador
Copy link
Owner

oblador commented Oct 2, 2021

This should be fixed by #505 AFAIK, can you can confirm @Vito96?

@Vito96
Copy link
Author

Vito96 commented Oct 4, 2021

Not totally, this PR (#505) of course optimizes the RSA check for the initial test using another key dimension, also properly uses the short condition evaluation here
Schermata 2021-10-04 alle 10 37 30

But it is useless evaluate isBiometry in this point (row 753)
Schermata 2021-10-04 alle 10 38 49

This PR can optimize the reorder of this lines for not compute del capability level for variant which isBiometrySupported is true and isBiometry is false.

Anyway, with the #505 PR, now the tests should be fine.

@Vito96
Copy link
Author

Vito96 commented Oct 21, 2021

Do you have any news about the approval?

@DorianMazur DorianMazur changed the title Optimized getCipherStorageForCurrentAPILevel method fix: optimize getCipherStorageForCurrentAPILevel method Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants