Skip to content

issue #1061 Port zxcvbnStrengthBar#1064

Merged
tomholub merged 7 commits intomasterfrom
ip-1061-port-zxcvbn
Mar 9, 2021
Merged

issue #1061 Port zxcvbnStrengthBar#1064
tomholub merged 7 commits intomasterfrom
ip-1061-port-zxcvbn

Conversation

@IvanPizhenko
Copy link
Contributor

close #1061

tomholub
tomholub previously approved these changes Mar 7, 2021
Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM - I'll wait for @DenBond7 to review tomorrow as well

@IvanPizhenko
Copy link
Contributor Author

I'll add a bit more tests for the random()

@IvanPizhenko
Copy link
Contributor Author

@tomholub Please recheck, I've added tests for random() and made a fix in it.

@Test
fun testBytesToPassword() {
val bytes = byteArrayOf(1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 10, 11, 12, 13, 14, 15)
assertEquals("1234-5678-90AB-CDEF", PgpPwd.bytesToPassword(bytes))
Copy link
Collaborator

@tomholub tomholub Mar 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DenBond7 this is what we use on browser extension when the provided private key is missing a pass phrase - we offer to auto-generate one. Is there a similar mechanism on Android yet? Or do we always ask user to provide a new one pass phrase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bytesToPassword() is not separate funciton in the original source code, but I've moved it here into the separate function to make it easier testable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DenBond7 this is what we use on browser extension when the provided private key is missing a pass phrase - we offer to auto-generate one. Is there a similar mechanism on Android yet? Or do we always ask user to provide a new one pass phrase?

We always ask a user to provide a pass phrase

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Eventually we may support this scenario to auto-generate for user. Not a priority for now, 2022

@IvanPizhenko
Copy link
Contributor Author

I've moved that to the package com.flowcrypt.email.security.pgp since I'm going to use that package again in the next task for things related to pgp.

@DenBond7
Copy link
Collaborator

DenBond7 commented Mar 8, 2021

I've moved that to the package com.flowcrypt.email.security.pgp since I'm going to use that package again in the next task for things related to pgp.

That's a good choice. Please use that package as a root place of things that relate to PGP.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IvanPizhenko Please don't save such changes. Android Studio and IDEA have different folders for settings. Just skip them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this one
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DenBond7 I'm using latest Android Studio for working in this repo. so not sure why that has happened.

@DenBond7
Copy link
Collaborator

DenBond7 commented Mar 8, 2021

@IvanPizhenko
I've refactored the code a little. Also, I've changed idents. Please change indents to the following

image

image

@tomholub
Copy link
Collaborator

tomholub commented Mar 8, 2021

Is the failed test still ok for merging?

@DenBond7
Copy link
Collaborator

DenBond7 commented Mar 8, 2021

Is the failed test still ok for merging?

Yes, it is. You can merge these changes. I've fixed tests, but the fixed code is stored in 1.1.6

@tomholub tomholub merged commit 77583b2 into master Mar 9, 2021
@DenBond7 DenBond7 deleted the ip-1061-port-zxcvbn branch March 11, 2021 07:01
@IvanPizhenko
Copy link
Contributor Author

IvanPizhenko commented Mar 11, 2021

@DenBond7 Such formatting settings can be saved per project and checked in into repo, so you don't need to to tune IDE-wide settings. Then, when you open project, setting become effective automatically, no matter what the IDE-wide settings are. This was already done in another repository and works good. I suggest to do the same here. I can prepare PR.

@IvanPizhenko
Copy link
Contributor Author

IvanPizhenko commented Mar 11, 2021

@DenBond7
My Andriod Studio version info:
Android Studio 4.1.2
Build #AI-201.8743.12.41.7042882, built on December 20, 2020
Registry: ide.new.welcome.screen.force=true, external.system.auto.import.disabled=true
Non-Bundled Plugins: org.intellij.plugins.markdown, org.jetbrains.kotlin, com.samdark.intellij-visual-studio-code-dark-plus

I've made a clean clone of this repo and opened it in the Android Studio, and here's what it has done:

 <project version="4">
   <component name="ProjectModuleManager">
     <modules>
-      <module fileurl="file://$PROJECT_DIR$/flowcrypt-android.iml" filepath="$PROJECT_DIR$/flowcrypt-android.iml" />
+      <module fileurl="file://$PROJECT_DIR$/.idea/modules/flowcrypt-android.iml" filepath="$PROJECT_DIR$/.idea/modules/flowcrypt-android.iml" />
       <module fileurl="file://$PROJECT_DIR$/.idea/modules/FlowCrypt/flowcrypt-android.FlowCrypt.iml" filepath="$PROJECT_DIR$/.idea/modules/FlowCrypt/flowcrypt-android.FlowCrypt.iml" />
     </modules>
   </component>
-</project>
+</project>

Can you please make sure you also have the same latest stable version of the Android Studio and try to do the same?
I believe this way we can find out what it should be.

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.

port zxcvbnStrengthBar

3 participants

Comments