Use only specified login strategy, if not "all"

[chuckwondo]

Fixes #945

Pull Request (PR) draft checklist - click to expand

• Please review our
  contributing documentation
  before getting started.
• Populate a descriptive title. For example, instead of "Updated README.md", use a
  title such as "Add testing details to the contributor section of the README".
  Example PRs: #763
• Populate the body of the pull request with:
  • A clear description of the change you are proposing.
  • Links to any issues resolved by this PR with text in the PR description, for
    example closes #1. See
    GitHub docs - Linking a pull request to an issue.
• Update CHANGELOG.md with details about your change in a section titled
  ## Unreleased. If such a section does not exist, please create one. Follow
  Common Changelog for your additions.
  Example PRs: #763
• Update the documentation and/or the README.md with details of changes to the
  earthaccess interface, if any. Consider new environment variables, function names,
  decorators, etc.

Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!

Pull Request (PR) merge checklist - click to expand

Please do your best to complete these requirements! If you need help with any of these
requirements, you can ping the @nsidc/earthaccess-support team in a comment and we
will help you out!

• Add unit tests for any new features.
• Apply formatting and linting autofixes. You can add a GitHub comment in this Pull
  Request containing "pre-commit.ci autofix" to automate this.
• Ensure all automated PR checks (seen at the bottom of the "conversation" tab) pass.
• Get at least one approving review.

---

📚 Documentation preview 📚: https://earthaccess--1129.org.readthedocs.build/en/1129/

[github-actions]

👈 Launch a binder notebook on this branch for commit 9d71e13

I will automatically update this comment whenever this PR is modified

👈 Launch a binder notebook on this branch for commit 0088923

👈 Launch a binder notebook on this branch for commit 06e9b29

👈 Launch a binder notebook on this branch for commit 3c8d4e1

👈 Launch a binder notebook on this branch for commit d39dda9

👈 Launch a binder notebook on this branch for commit e2c577d

👈 Launch a binder notebook on this branch for commit 7875fad

[mfisher87]

I think we should remove the default indicators from this docstring as those are specified and generated by the type annotations already:

We're likely to let this information fall out of sync if we duplicate it.

[chuckwondo]

Agreed. Will do.

[mfisher87]

It's possible some may rely on this implicit/automatic login behavior. I don't think we should necessarily keep this behavior around, but I do think we should call this out as a breaking change in the changelog if we're removing it.

And let's also update the title of this PR to mention the removal of automatic login behavior!

[chuckwondo]

Yes, it is possible, although I would be very surprised if this broke anybody's code. I don't think I've ever seen any example code where someone did not include a call to earthdata.login().

Regardless, I agree that this does represent a breaking change, as it is certainly possible that someone is (unknowingly) relying upon the (undocumented?) automatic login behavior to successfully download files.

I will also comb the docs to see if we actually document the automatic login behavior. I don't think we do, but if we do, I'll need to update the docs as part of this PR.

[chuckwondo]

@mfisher87, I updated the changelog with the "breaking change" called out. Let me know how that reads (I simply used the format suggested in common changelog conventions), and if anything else needs tweaking.

[mfisher87]

The change looks good to me, but I'm curious if you feel we have consensus around removing automatic login behavior. It sounds good to me, but I haven't asked around about it.

[chuckwondo]

The change looks good to me, but I'm curious if you feel we have consensus around removing automatic login behavior. It sounds good to me, but I haven't asked around about it.

Fair question. I've copied others to solicit more input, so I won't merge this PR yet, but here's my take on it all:

Unfortunately, unless I'm missing something, I don't think there's any completely non-breaking way to address the myriad issues we have around auth.

In this case, if we leave the auto-login behavior in place, we remain unable to address the issue this PR resolves. That is, with auto-login as it currently stands, we cannot prevent, for example, attempting login (implicitly) via env vars or netrc when the user specifically wants to ignore them and (explicitly) force only prompting the user for creds.

Of course, that's just one example. Generally speaking, I find this implicit, non-overrideable behavior highly problematic (and may very well be the source of other auth issues/bugs in our backlog).

I believe keeping this implicit behavior is worse than the alternative, which is to remove the auto-login behavior and run the risk of breaking existing code.

In the case of removing the auto-login behavior, there are only 2 scenarios where this may have an impact:

1. search: All of our search_* functions end up causing auto-login (because they all access earthaccess.__auth__, which is what triggers the behavior). The worst case scenario created by removing auto-login is that search results might contain fewer results, but only in cases where specific search criteria would include results available only to certain users. This almost certainly won't break code (i.e., won't raise an exception that would otherwise not be raised), but rather might produce different results.
2. read: This is the more concerning scenario. Without being logged in, any attempt to read data will result in an auth exception.

Anecdotally, I suspect most users always call earthdata.login first (at least before attempting to read, if not also before attempting to search), so I also suspect there will be few issues opened as a result of problems encountered by removing the auto-login behavior.

However, of the few (I suspect) such breaking scenarios that do arise, none (I suspect) will be related to search, and thus all will be related to read scenarios where users "forget" to call earthdata.login before reading. If the error that the user encounters isn't enough for them to instantly realize for themselves that they must add a call to earthdata.login, we should be able to quickly help them resolve it.

I have not recently looked at our docs, so this may be well covered, but if and when we do see users raising issues due to removal of auto-login, we may want to review our docs to see if we can improve the explicitness and clarity of our docs on auth/login.

To our benefit in removing this auto-login behavior, the behavior is not documented anywhere (as far as I could tell), so I would be very surprised if anybody is even aware that this behavior exists. (I certainly didn't realize it until recently, and I've even read that very code many times, but never had it sink in that auto-login was happening. Maybe I just have a thicker skull than most.)

Longer term, I want to incrementally continue improving our auth process to further eliminate any remaining implicit behavior that may be problematic, and provide more explicit controls/overrides for users, and relatedly with respect to session management.

cc: @betolink, @jhkennedy, @itcarroll

[itcarroll]

I agree with @chuckwondo. Thanks for all this progress.

[betolink]

Thanks Chuck! I've used the implicit behavior knowing it's there and I think some users will run into this but with a clear explanation I think we are OK.

[mfisher87]

I'm 100% sold on this change, but I do want to say I'm most worried about the latter part of this statement:

This almost certainly won't break code (i.e., won't raise an exception that would otherwise not be raised), but rather might produce different results

If any users are relying on implicit login to turn up restricted data as part of their search, this could lead to a sneaky bug. Ideally, they're only searching for restricted data, and so they go from N results to 0 results, and they'd instantly know there's a problem. But if they are searching for both restricted and unrestricted data, they would go from N to <N results and their analysis may be unknowingly incorrect. Maybe the best thing we can do about this is (1) provide a (very simple) migration guide and (2) announce the release on Zulip and Slack with a warning and link to the migration guide.

[chuckwondo]

I'm 100% sold on this change, but I do want to say I'm most worried about the latter part of this statement:

This almost certainly won't break code (i.e., won't raise an exception that would otherwise not be raised), but rather might produce different results

If any users are relying on implicit login to turn up restricted data as part of their search, this could lead to a sneaky bug. Ideally, they're only searching for restricted data, and so they go from N results to 0 results, and they'd instantly know there's a problem. But if they are searching for both restricted and unrestricted data, they would go from N to <N results and their analysis may be unknowingly incorrect. Maybe the best thing we can do about this is (1) provide a (very simple) migration guide and (2) announce the release on Zulip and Slack with a warning and link to the migration guide.

Agreed. This is perhaps the worse of the 2 cases because of its subtlety. At least the other case will be immediately obvious by raising an exception.

Hopefully this "reduced search results" case will be a rare case, but I agree that we should provide greater visibility and clarity around this change along the lines of your suggestions to reduce the likelihood of surprises without having a place to point folks to enable quick rectification.

Would you be open to allowing this PR to land (without the guide) so that it allows tackling other auth issues possibly in parallel to developing the short migration guide you're suggesting? If so, would you mind opening a new "docs" issue for the guide and coordinating announcement?

We could hold off on any new version release until the migration guide also lands.

[chuckwondo]

Thanks Chuck! I've used the implicit behavior knowing it's there and I think some users will run into this but with a clear explanation I think we are OK.

@betolink, I would say that you might not be a representative user in this case because you're the one who wrote it (and thus are certainly aware of the undocumented behavior) 😄

[mfisher87]

Would you be open to allowing this PR to land (without the guide) so that it allows tackling other auth issues possibly in parallel to developing the short migration guide you're suggesting? If so, would you mind opening a new "docs" issue for the guide and coordinating announcement?

Sounds good, on it!