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

Added basic support for temporary urls #13

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

wachsylon
Copy link

No description provided.

Copy link
Collaborator

@d70-t d70-t left a comment

Choose a reason for hiding this comment

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

Thank you very much for this PR! 🎉
I've a few minor comments annotated to the code.
Could you also format the code using black (i.e. as outlined in the readme)?

swiftspec/core.py Outdated Show resolved Hide resolved
swiftspec/core.py Outdated Show resolved Hide resolved
swiftspec/core.py Outdated Show resolved Hide resolved
swiftspec/core.py Outdated Show resolved Hide resolved
swiftspec/core.py Outdated Show resolved Hide resolved
@wachsylon
Copy link
Author

@d70-t I think I got everything.

@d70-t
Copy link
Collaborator

d70-t commented Jul 21, 2023

Thanks @wachsylon for the updates.
I think 1fa8dca is fine, however, I'm a bit puzzeled by the purpose of 2ca48a8. Yes, the auth object is a list, such that we can potentially load different ways to authenticate depending on which url we are trying to access, but it only seems to be reasonable to have one authentication method per url. The change possibly creates two auth-list entries, but they both are for the same url = os.environ.get("OS_STORAGE_URL") and thus the second entry should never be taken.

While writing this, I see that as it's currently implemented, there is indeed a possibility that both mechanisms are taken (headers_for_url might match on the first and params_for_url might match on the second). My feeling is, that this really shouldn't be the case, because I don't see a useful case in which two authentication methods are mixed.

@d70-t
Copy link
Collaborator

d70-t commented Jul 21, 2023

Thinking a bit more about this, it might even be better to have something like:

def headers_and_params_for_url():
    # search though `auth` and once an auth method for that url is found, determine whatever headers and params are needed to authenticate against that url
    return headers, params

Such that we can avoid the case where we accidentally decide for two different authentication methods in headers_for_url and params_for_url.


def headers_for_url(self, url):
headers = {}
for auth in self.auth:
if url.startswith(auth["url"]):
if url.startswith(auth["url"]) and "token" in auth:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change probably shouldn't be there in this form. The loop over all the auth methods should stop as soon as there is a match on the url. It might then be the case, that for the matched auth method, the headers should be empty (because it's params-based).

(it's the same issue for the loop in params_for_url)

Copy link
Author

Choose a reason for hiding this comment

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

I think if you have a url AND a token, that should always be preferred, right? however I do not understand why token should not be checked.

Copy link
Author

Choose a reason for hiding this comment

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

@d70-t i do not understand how to decide what to take unless using the check for token

Copy link
Collaborator

Choose a reason for hiding this comment

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

My thinking was that auth would be a list of authentication methods and the rule would be to apply the first method which matches on the requested url. Along this line, checking for token would be a way of skipping the first entry matching the url (but we don't want to skip).

At some point, I thought if auth might be better implemented as a dict, but that doesn't support prefix-matching, thus it became a list.

Copy link
Collaborator

@d70-t d70-t Jul 24, 2023

Choose a reason for hiding this comment

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

@d70-t i do not understand how to decide what to take unless using the check for token

It would be up to the method creating the auth list, to ensure only the right option (token or temporary) is part of the list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I made a draft of this line of thought here. It passes the unit tests, but as (unfortunately) authentication isn't yet covered by tests, I don't know yet if it really works.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds all good. I think and hope your draft does the same as my code so i dont care what implementation we use. if there are other params or header options that we want to include someday it could be nice to already have separate functions for each. Also, if the authentication does not care about providing two ways (params and headers), we could just provide both.

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.

2 participants