Refactor auth logic

[forsyth2]

Issue resolution

• Closes [Feature]: Add refresh tokens for better Globus functionality #338
• Closes zstash Globus functionality has become overly cumbersome #339

Select one: This pull request is...

• a bug fix: increment the patch version
• a small improvement: increment the minor version
• an incompatible (non-backwards compatible) API change: increment the major version

1. Does this do what we want it to do?

Objectives:

• Simplify using Globus inside zstash. As pointed out in zstash Globus functionality has become overly cumbersome #339, the introduction of Globus consents authentication a year ago has ultimately caused a 2-step process to grow to 7 steps. Can we return it to 2 steps?
• Simplify the Globus logic. As described in [Feature]: Add refresh tokens for better Globus functionality #338, it should more closely match the example code at https://globus-sdk-python.readthedocs.io/en/stable/examples/native_app.html
• Extend the life of a Globus transfer, with refresh tokens. Described in [Feature]: Add refresh tokens for better Globus functionality #338.

Required:

• Product Management: I have confirmed with the stakeholders that the objectives above are correct and complete.
• Testing: I have added at least one automated test. Every objective above is represented in at least one test.
• Testing: I have considered likely and/or severe edge cases and have included them in testing.

If applicable:

• Testing: this pull request adds at least one new possible command line option. I have tested using this option with and without any other option that may interact with it.

2. Are the implementation details accurate & efficient?

Required:

• Logic: I have visually inspected the entire pull request myself.
• Logic: I have left GitHub comments highlighting important pieces of code logic. I have had these code blocks reviewed by at least one other team member.

If applicable:

• Dependencies: This pull request introduces a new dependency. I have discussed this requirement with at least one other team member. The dependency is noted in zstash/conda, not just an import statement.

3. Is this well documented?

Required:

• Documentation: by looking at the docs, a new user could easily understand the functionality introduced by this pull request.

4. Is this code clean?

Required:

• Readability: The code is as simple as possible and well-commented, such that a new team member could understand what's happening.
• Pre-commit checks: All the pre-commits checks have passed.

If applicable:

• Software architecture: I have discussed relevant trade-offs in design decisions with at least one other team member. It is unlikely that this pull request will increase tech debt.

[forsyth2]

I've tried to do a preliminary code change here to address #338 and #339. However, the code in this pull request unfortunately results in python -u -m unittest tests/test_globus.py just hanging after prompting for one auth code. I will continue looking into this. In the meantime:

• @mahf708 @lukaszlacinski do you have comments/suggestions for improving/properly implementing this pull request?
• @lukaszlacinski what was the design decision reasoning to use the fair_research_login.NativeClient rather than the globus_sdk.NativeAppAuthClient?

To @golaz's point in #339, the Globus authentication process has really impacted zstash's usability. We should address it before the next E3SM Unified release. Examples of impacts:

• zstash Globus functionality has become overly cumbersome #339 -- As mentioned in the PR description, a 2-step process is now 7 steps
• How do I run the Globus unit test? #329 -- Any time I even want to run the Globus test, I have to follow an entire sequence of instructions listed here.

The two biggest problems at the moment seem to be:

1. To use Globus in any zstash production use case requires doing a toy problem with zstash to get the authentications sorted out first.
2. Losing authentication in the middle of a multi-day transfer.

[mahf708]

My main recommendation is completely getting rid of fair_research_login; also, the test tests/test_globus.py seems to be independent of zstash/globus.py, which means changing one isn't necessarily going to be reflected in the other. I am misreading this? If my understanding is correct, the test has to be updated, right?

The fair_research_login stuff seems to still be in use here:

• zstash/zstash/globus.py

  Line 67 in 1aa459e

  native_client = NativeClient(

• zstash/tests/test_globus.py

  Lines 56 to 57 in 1aa459e

  	native_client = NativeClient(
  	client_id="6c1629cf-446c-49e7-af95-323c6412397f",

[mahf708]

Additionally, once you get rid of fair_research_login, be sure to relax the constraint on the globus sdk:

zstash/conda/meta.yaml

Line 25 in 1aa459e

- globus-sdk >=3.0.0,<4.0.0

[forsyth2]

the test tests/test_globus.py seems to be independent of zstash/globus.py

Yes, the test function preactivate_globus replaces the production-use function globus_activate.

Details, going through the test code:

testLs calls helperLsGlobus

helperLsGlobus calls self.preactivate_globus()

helperLsGlobus then sets up the HPSS path, the cache, and the directory setupDir to archive.

helperLsGlobus then calls self.create() which will run zstash create. This will trigger Globus because hpss_path was set as f"globus://{hpss_globus_endpoint}/~/zstash_test/" in testLs. That is, it will ultimately call globus.py globus_transfer. Now, globus_activate("globus://" + remote_ep) will not be called because test_globus.py preactivate_globus already set self.transfer_client = TransferClient(authorizer=transfer_authorizer).