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

Refactor 2fa #105

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor 2fa #105

wants to merge 1 commit into from

Conversation

stelle11
Copy link

@stelle11 stelle11 commented Oct 9, 2022

I did a complete refactoring of the 2fa authentication because of multiple issues I had related to #96.
I was only using yahoo for my tests but I did multiple different tests that also covered the issue desribed in #96. These changes fixes #96 in my opinion.

The issues I had with the existing code was:

  • No deletion of the authentication mails received every 6 minutes when pyaarlo couldn't log in
  • If multiple authentication mails was in the inbox it would always only read the first received mail (not the newest)
  • The 2fa authentication could not read the code in the mail correctly - 2FA not parsing mail properly? #96

Quick description of the refactoring changes:

  • Get mail ids for all mails in the inbox
  • Only take the last 10 mail ids
  • If any of them (starting from the newest) is from Arlo then try to read the code and return it
  • Otherwise wait for a new mail to arrive then check latest 10 mails again

I did the following tests after having everything fully implemented:

  • Waiting for the authentication mail with no other authentication mails in the inbox
  • Waiting for the authentication mail with more than one other authentication mail in the inbox
  • Read the latest authentication mail with no other authentication mails in the inbox
  • Read the latest authentication mail with more than one other authentication mail in the inbox

I did make some changes to the code like instead of using imaplib's search function to find all mails from do_not_reply@arlo.com, I changed to get all mail ids and check the sender of the last 10 mails. The reason being that the search function did not return all mails from do_not_reply@arlo.com. The search function does not work if there's more than 10K mail ids - at least not with yahoo.

@twrecked
Copy link
Owner

I'll give this a try here with gmail and apple.

One thing, it's removing the SSL options change and read-only change from this commit: #104

@Jopand
Copy link

Jopand commented Nov 5, 2022

Tried out this change. At first I thought this PR fixed the #96 issue, but it turned out I was fooled by an existing token. When that ran out, I couldn't do a new 2fa login. So I reverted to the fix I mention in #96.

def is_arlo_mail(self, mail_id):
self._arlo.debug("2fa-imap: Checking if mail is Arlo mail, mail id: " + format(mail_id))
res, msg = self._imap.fetch(mail_id, "(RFC822)")
if isinstance(msg[0][1], bytes):
Copy link

@Jopand Jopand Nov 10, 2022

Choose a reason for hiding this comment

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

For some reason, I have to use msg[1][1] in line 131 and 132 with my yahoo mail. Mentioning it in #96. No idea why it works differently if you've also tested with a yahoo mail

Copy link

Choose a reason for hiding this comment

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

I made a printout of msg[0][1] and msg[0][1]..

msg[0][1]: 50

... nothing else...

msg[1][1]:
b'Received: from 127.0.0.1\r\n by atlas-production.v2-mail-prod1-gq1.omega.yahoo.com pod-id atlas--production-gq1-6db95f6ddc-xz57w.gq1.yahoo.com with HTTP..... + a lot more. LOOOOOONG string containing the entire mail

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.

2FA not parsing mail properly?
3 participants