-
Notifications
You must be signed in to change notification settings - Fork 107
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
Modularizing authentication #433
Comments
Thanks for this great writeup @rwegener2! Of the two solutions you propose, I would definitely lean towards the second one. We actually not too recently removed our own
The short answer is this is a legacy from the early days of programmatic data access. In many cases, particularly for tutorials, the primary mode of authentication was interactive (and there was no icepyx or earthaccess wrapping the manual sending of auth info with a
I think this is the way to go, and I think we could implement it without changing the user experience. For classes that might need auth, we can include it as part of the init as you showed in the Variables module in your linked commit. Rather than directly passing in a |
The Problem
The authentication method that exists right now is inherently tied to the
Query
class because the.earthdata_login()
method is a method on that class. That has led to passing._session
values fromQuery
objects toVariables
objects as needed.As the cloud read effort develops, additional modules, such as
Read
, will also need to authenticate. Because of this I would like to propose that we make authentication in the repository more modular, so it can be more easily extended to different classes.Possible Solutions
Both of the ideas below create an
EarthdataAuth
class. This class would have the existingearthdata_login()
functionality within it.1 - Inheritance
We could create an class called
EarthdataAuth
that holds the._auth
,._session
, and._s3login_credentials
attributes that currently get created inQuery
.Query
,Variables
, and other classes could then be subclasses of theEarthdataAuth
superclass.Pros/Cons: After some reading it seems that isn't exactly the type of relationship we want to convey. The set of complexity added by a new level of inheritance feels disproportionate to the fairly simple need to keep track of 2-3 attributes and one function. The pro of this solution, however, is that I believe we could it up such that nothing about the user experience changes.
2 - Creating
EarthdataAuth
as a class that other objects have as an attributeQuery
and other related objects could have an.auth
attribute that is an instance ofEarthdataAuth
.EarthdataAuth
can get created either by calling theearthdata_login()
function, or automatically when a user needs authentication. The user (or the internal code) would access attributes likeipx.Query.auth._session
.Pros/Cons: I think this solution could simplify our internal code and at least for me it is more intuitive than inheritance (that may just be me, though). The con of this solution is that it would change how the user authenticates, either by removing that step all together or by requiring they switch to
ipx.Query.auth.earthdata_login()
.Questions
earthdata_login()
function themself? Are we interested in setting up the workflow such that the user is prompted for login credentials only when the execute a function that requires authentication? I think that could happen fairly easily on our end, but it would change the user experience.Code
move_auth
branch shows a wip example of approximately what the inheritance method could look like (method no. 1 above). I got that far and had some issues. I think the ordering of classes was causing different ._session objects to overwrite each other. That's when I starting thinking about if method no. 2 above would be simpler.The text was updated successfully, but these errors were encountered: