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.

Sign up for GitHub

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

core: add missing prefix property to auth backend #165

Merged
merged 3 commits into from
Oct 19, 2024
Merged

core: add missing prefix property to auth backend #165

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7092f16
core: add missing prefix property to auth backend
tarilabs Oct 17, 2024
206b262
implement review feedback
tarilabs Oct 19, 2024
18c964b
update changelog and version
tarilabs Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion oras/auth/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@ class AuthenticationException(Exception):
pass


def get_auth_backend(name="token", session=None, **kwargs):
def get_auth_backend(name="token", session=None, prefix="https", **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

@tarilabs this won't always be https - specifically in testing. I think we'd want to inherit this from the parent that created it, and use the same convention there with insecure as a boolean to determine the prefix (and not hard code http or https). Let me know your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line is specifically about the default isn't it,
and is mainly never used because here:

https://github.com/oras-project/oras-py/pull/165/files#diff-ccb407ceff93417721d33c41bf1975c04c98454fd2a36518a7b8572808db03a4R81

is set explicitly.

am I missing something? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think you want to set it based on the insecure parameter:

 self.prefix: str = "http" if insecure else "https"

Which is in the init of the provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm, I've actually mimic-ed on existing code and did that in line 23 of this file.

making the line you quote:

https://github.com/oras-project/oras-py/pull/165/files#diff-fcbd6629a667f0deb29e01bccd3fa21f8790961f48092d8cee24ad00986706a9R22

another (unused) default.

I'm happy to refactor this code differently if that helps, I was just trying to go with the existing "code convention"? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

step 2.

unless otherwise specified, this function get_auth_backend() parameter prefix defaults to https; but for all usages in oras code, is set from provider's prefix, as we seen in step1.

backend = auth_backends.get(name)
if not backend:
raise ValueError(f"Authentication backend {backend} is not known.")
backend = backend(**kwargs)
backend.session = session or requests.Session()
backend.prefix = prefix
Copy link
Contributor Author

Choose a reason for hiding this comment

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

step 3.
the determined backend (either token or basic) get .prefix property set to the value received in step 2

this maintains code-style for .session another property set on determined backend.

return backend
1 change: 1 addition & 0 deletions oras/auth/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class AuthBackend:

def __init__(self, *args, **kwargs):
self._auths: dict = {}
self.prefix: str = "https"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

step 4.

the base AuthBackend by default would have a property prefix valorized to https, but as we just seen in step 3, this is set to the actual prefix value.


def get_auth_header(self):
raise NotImplementedError
Expand Down
2 changes: 1 addition & 1 deletion oras/provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def __init__(
self.session.cookies.set_policy(DefaultCookiePolicy(allowed_domains=[]))

# Get custom backend, pass on session to share
self.auth = oras.auth.get_auth_backend(auth_backend, self.session)
self.auth = oras.auth.get_auth_backend(auth_backend, self.session, self.prefix)
Copy link
Contributor Author

@tarilabs tarilabs Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

step 1.

the prefix of provider is set a few lines above:

oras-py/oras/provider.py

Line 69 in 36ef98a

self.prefix: str = "http" if insecure else "https"

and is "pass forward" as suggested to the call of oras.auth.get_auth_backend()


def __repr__(self) -> str:
return str(self)
Expand Down
Loading