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

Add security code hash to magic code SMS for native Android #15342

Closed
mountiny opened this issue Feb 22, 2023 · 36 comments
Closed

Add security code hash to magic code SMS for native Android #15342

mountiny opened this issue Feb 22, 2023 · 36 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering FirstPick Engineering only, please! Only add when there is an identified code solution. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@mountiny
Copy link
Contributor

mountiny commented Feb 22, 2023

Problem

We are going passwordless and we are using magic links and magic codes to sign in users. This feature is now under beta and its being slowly rolled out. We have implemented the SMS code autofill for magic code for all available platforms except Android Native where it requires a backend change #14853

Why is it important

This is improving UX and sets NewDot on par with other apps using similar security passcodes and consistency across the platforms.

Solution

As @narefyev91 detailed in this comment, we will have to add a specific Hash to the SMS message with magic code if it has been triggered from native Android.

There are some details about the hash here. To generate hash you can use this sh file - command to execute ./sms_retriever_hash_v9.sh --package com.expensify.chat --keystore ./android/app/debug.keystore (for debug keystore hash = h7MoDsTCzjw).

In PHP, we will have to check if the request is coming from a native Android device and in such case, we need to update the content of the message to include the hash detailed above.

@narefyev91 will be the best person to ask for help regarding the hash

Task (Added by @hayata-suenaga)

As this issue needs several PRs and deployment of these PRs needs to be coordinated, I list these PRs here

The deployment schedule should be App, Auth -> Web-E.
This means that Web-E should be held until App and Auth PRs are deployed to production. App and Auth PRs can be deployed independent of each other.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01078cb4f203d82721
  • Upwork Job ID: 1628334514741329920
  • Last Price Increase: 2023-02-22
@mountiny mountiny added Engineering Daily KSv2 FirstPick Engineering only, please! Only add when there is an identified code solution. Internal Requires API changes or must be handled by Expensify staff Bug Something is broken. Auto assigns a BugZero manager. labels Feb 22, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 22, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~01078cb4f203d82721

@MelvinBot
Copy link

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@MelvinBot
Copy link

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@MelvinBot
Copy link

Triggered auto assignment to Contributor Plus for review of internal employee PR - @sobitneupane (Internal)

@mountiny
Copy link
Contributor Author

Asking internally who wants to pick this up

@melvin-bot melvin-bot bot added the Overdue label Feb 24, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 24, 2023
@mallenexpensify
Copy link
Contributor

@mountiny do you have a link to where the convo is happening?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Feb 25, 2023
@mountiny
Copy link
Contributor Author

Asked here, nobody picked this up yet.

@MelvinBot
Copy link

@mallenexpensify, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot removed the Overdue label Mar 1, 2023
@hayata-suenaga
Copy link
Contributor

@mallenexpensify I saw this is a first pick issue. I'd like to take this issue

@melvin-bot melvin-bot bot added the Overdue label Mar 3, 2023
@hayata-suenaga hayata-suenaga self-assigned this Mar 3, 2023
@hayata-suenaga
Copy link
Contributor

Asked a question on slack

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 3, 2023
@hayata-suenaga
Copy link
Contributor

I'm still working on the PR

@melvin-bot melvin-bot bot removed the Overdue label Mar 6, 2023
@hayata-suenaga
Copy link
Contributor

seeking help with getting access to production deployment certification (which is needed to compute the hash) in slack.

@roryabraham roryabraham self-assigned this Mar 9, 2023
@roryabraham
Copy link
Contributor

Self-assigning to help unblock @hayata-suenaga by generating the hash

@roryabraham
Copy link
Contributor

Ok @hayata-suenaga, the production hash is h7MoDsTCzjw

@roryabraham roryabraham removed their assignment Mar 9, 2023
@hayata-suenaga
Copy link
Contributor

hayata-suenaga commented Mar 9, 2023

@roryabraham I think the hash you posted h7MoDsTCzjw might have created using the debug keystore instead of the production keystore. That is the same hash that @vit posted here. It's also the same hash I got when I ran the hash creation script on my local computer.

I'd appreciate it if you could double check it when you have time.

@roryabraham
Copy link
Contributor

Hmmm... that is weird. I'm getting the same hash with the debug keystore and the one I downloaded from 1P.

@roryabraham
Copy link
Contributor

I have decrypted the production keystore directly using this line of code, then computed the hash again, and got the same result. So it seems like that hash is correct? Weird.

@hayata-suenaga
Copy link
Contributor

@roryabraham thank you so much for confirmation. are we using the same keystore for debug and production then???

@AndrewGable
Copy link
Contributor

@roryabraham and I were able to confirm the production hash is: xy9/YUBP8xj

We were not 100% able to confirm this, but we think the dev hash is: VdYMaWKQlXp

Please let us know if you need any more help with the hashes 👍

@hayata-suenaga
Copy link
Contributor

hayata-suenaga commented Mar 15, 2023

Thank you Andrew for double checking this

so there is two possibilities for the dev hash somehow, h7MoDsTCzjw here and VdYMaWKQlXp here.

I'll mention both hash on my PR so people can try both when testing.

@MelvinBot
Copy link

@mallenexpensify, @sobitneupane, @hayata-suenaga Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@hayata-suenaga
Copy link
Contributor

PRs are under review now

@MelvinBot
Copy link

@mallenexpensify, @sobitneupane, @hayata-suenaga Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@hayata-suenaga
Copy link
Contributor

PRs are still under review

@MelvinBot
Copy link

@mallenexpensify, @sobitneupane, @hayata-suenaga Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@hayata-suenaga hayata-suenaga added Reviewing Has a PR in review and removed Reviewing Has a PR in review labels Apr 7, 2023
@melvin-bot melvin-bot bot added the Overdue label Apr 7, 2023
@hayata-suenaga
Copy link
Contributor

Waiting for the PR to be deployed to production to do QA.

@melvin-bot melvin-bot bot removed the Overdue label Apr 7, 2023
@MelvinBot
Copy link

@mallenexpensify, @sobitneupane, @hayata-suenaga Huh... This is 4 days overdue. Who can take care of this?

@hayata-suenaga
Copy link
Contributor

will test again this today

@mountiny
Copy link
Contributor Author

Have you been able to test this?

@hayata-suenaga
Copy link
Contributor

@mountiny yes I was able to confirm the autofill option appears when I log in from Android.

image000000

@mountiny
Copy link
Contributor Author

@hayata-suenaga great job! Thanks for sticking with this!

@mountiny
Copy link
Contributor Author

I think we can close this, right, or @sobitneupane are you awaiting any payment in here, I think there was no App pr

@sobitneupane
Copy link
Contributor

Good to close. No payment for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering FirstPick Engineering only, please! Only add when there is an identified code solution. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants