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 2FA test and 2FA support for integration tests #51

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

Conversation

FranciscoPombal
Copy link

@FranciscoPombal FranciscoPombal commented Aug 9, 2024

Note: description updated according to updates below.


  • Credentials for non 2FA/MFA-enabled accounts as well as 2FA/MFA-enabled accounts are supplied from the environment.

  • For tests that can use both types of credentials, credentials for non 2FA/MFA-enabled accounts are preferred, if available.

  • Tests that require one or the other type of credentials/accounts are skipped if the required type of credentials is not available.

  • For the tests, a single MFA and non-MFA login is performed at the start of testing, and any subsequent logins for each test are done with session token logins.

  • A new test-only dependency is introduced for generating 2FA/MFA codes from their secrets: github.com/pquerna/otp@v1.4.0.

  • golang.org/x/crypto has been bumped to the latest version.

  • Dropped usage of io/ioutil.


This is not ready because I cannot get the tests to pass properly.
With a 2FA/MFA-enabled account, I can get all tests to pass except one if I:

  • Run them one at a time.
  • Wait a bit between running each test (a few tens of seconds).
  • Clear the login sessions created by the tests in the MEGA web app's "Session history" interface between each test.

When these workarounds are not all performed, the tests will fail with the following error: The upload target URL you are trying to access has expired. Please request a fresh one.
Additionally, TestEventNotify is not passing no matter what, with the same error.

I have not tested with a non 2FA/MFA-enabled account.


Currently my knowledge of MEGA's API and SDK internals is limited, so any help fixing these issues is appreciated.

I would expect we need to at least clean up sessions between tests (perform a logout).
A logout is performed on testing teardown.


Test setup to replicate the above results (for a 2FA/MFA-enabled account):

  1. Export these variables to the environment:

    MEGA_USER_MFA=foobar@example.com
    MEGA_PASSWD_MFA=foobar123
    MEGA_SECRET_MFA=SECRETTOKEN2FAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA====
    
  2. Run tests.

    Note that you may need to increase go test's -timeout, especially if the MEGA account used for testing has many files, because for such accounts it may take a long time to login and/or perform other operations under test.

- Credentials for non 2FA/MFA-enabled accounts as well as
 2FA/MFA-enabled accounts are supplied from the environment.

- For tests that can use both types of credentials, credentials for non
 2FA/MFA-enabled accounts are preferred, if available.

- Tests that require one or the other type of credentials/accounts are
 skipped if the required type of credentials is not available.

- A new test-only dependency is introduced for generating 2FA/MFA codes
 from their secrets: `github.com/pquerna/otp@v1.4.0`.

- `golang.org/x/crypto` has been bumped to the latest version.
@ncw
Copy link
Collaborator

ncw commented Aug 12, 2024

I would expect we need to at least clean up sessions between tests (perform a logout).

We really should be doing this anyway as the sessions just pile up. Rclone now has a Shutdown method which could be used for this.

Currently my knowledge of MEGA's API and SDK internals is limited, so any help fixing these issues is appreciated.
I would expect we need to at least clean up sessions between tests (perform a logout).

My knowledge is limited too! I took on a maintainer role for this repo so I could keep it working with rclone, but I don't have time to dig into the SDK to find out how stuff works. I think you know more about it than me now!

These tests look safe to merge as they won't run unless we have the correct env var.

- Move command definitions to a separate file.

- Clean up sessions in most cases in tests with the new logout command.
Sessions are only properly terminated in successful runs of tests.
@FranciscoPombal
Copy link
Author

OK I've implemented the logout command.
Now tests won't leave sessions behind, if they finish successfully, and if the logout request at the end of each test is performed successfully.
Tests are still leaving behind test files and directories, though.
Session management in tests can be made more robust still, but it's a start.

Additionally, I now understand why the tests are not passing unless run one at a time, waiting some time between each test.

At least with MFA-enabled accounts, the current way go-mega does logins will cause all logins after a first one to fail if the following logins are done with the same 2FA code as the first one, even if the initial session is terminated properly (logged out).
This means one must wait at most 30 seconds before running the next test (the time for a 2FA code to expire).

I need to look at the SDK and official apps/web app some more to figure out how they handle the login flow to account for this.

@ncw
Copy link
Collaborator

ncw commented Sep 17, 2024

Great progress @FranciscoPombal :-)

Let me know when you think you'd like to merge what you've done.

@FranciscoPombal
Copy link
Author

Thanks!
I have submitted an issue to the MEGA SDK repo with my findings of what I believe to be an issue in the SDK or maybe server-side.
If they address this it would make our lives a lot easier.

Regardless, I have an idea for how to work around the remaining blockers for this PR even if nothing comes out of that report; we'll see.

It's the `sek` field in login responses.
This is necessary for the upcoming "dump session" implementation.
The session dump string is the same as one obtained from running
the `session` command of `mega-cmd`.

Currently, only session version 1 (normal login session) is supported.
- This enables tests to run (and pass) without having to worry about
  expired or invalid sessions due to fully logging in/out too quickly.

- This works by performing one full login as part of the setup of the
  test suite, and performing session logins for each test.

- Login/Logout code is exercised as part of the setup/teardown
  of the test suite.

- A `session.txt` file with the session string is created as part of the
  setup and removed on teardown.

- `TestEventNotify` is skipped for now.
@FranciscoPombal
Copy link
Author

FranciscoPombal commented Sep 20, 2024

OK, I've implemented session logins to work around the login issues.

Tests are all passing now, except for one that is explicitly skipped, TestEventNotify, which I've noted in the relevant commit message.
I think it should be dealt with later.

So this PR is ready!

@FranciscoPombal FranciscoPombal marked this pull request as ready for review September 20, 2024 10:10
@FranciscoPombal
Copy link
Author

I've just noticed that as it stands, this PR supersedes #36 (thus closing #35) and #47.

#30 also appears to have been obsoleted already.
#25 should be closed as well, since 2FA login is already supported by this library; with regards to rclone specifically, it duplicates rclone/rclone#3165.

@FranciscoPombal
Copy link
Author

@ncw ping

@vinibali
Copy link

Wow, great results!

@bmacphail
Copy link

I've been following the request for this feature in the rclone issue for literally years! Thanks so much for your work on this @FranciscoPombal!!

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.

4 participants