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

pass session object to client #597

Closed

Conversation

mishaschwartz
Copy link
Contributor

Add option to make requests with WeaverClient using a requests.Session instance.

This allows requests to be made with the session cookies that may already be attached to this requests.Session instance.

Useful as an alternative to authenticating with an auth argument when a user has already authenticated and they have access to the session cookie. This is mostly so that we can take advantage of this feature with weaver bird-house/birdhouse-deploy#407

@fmigneault
Copy link
Collaborator

Comment on lines 1872 to +1874
@cache_region("request")
def _request_cached(method, url, kwargs):
# type: (AnyRequestMethod, str, RequestCachingKeywords) -> Response
def _request_cached(method, url, kwargs, session):
# type: (AnyRequestMethod, str, RequestCachingKeywords, Optional[requests.Session]) -> Response
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if the session object is serializable for the caching decorator.

Would also avoid the extra parameter. Everything should be in RequestCachingKeywords.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if the session object is serializable for the caching decorator.

Local tests seem to show that it caches it ok.

Everything should be in RequestCachingKeywords

Are you sure? These get passed directly to session.request in the underlying function and that doesn't have a session parameter. So we'd have to factor it out later on.

Comment on lines +331 to +332
def __init__(self, url=None, auth=None, session=None):
# type: (Optional[str], Optional[AuthHandler], Optional[Session]) -> None
Copy link
Collaborator

Choose a reason for hiding this comment

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

See PR comment about using other AuthHandler alternatives. Seems to me that available ones are sufficient to handle the use case (passing Magpie cookies).

If they are still not sufficient for some reason (please justify if so), I would rather have a SessionAuthHandler that reuses the same auth parameter instead of adding a new one only for that use case.

Copy link
Contributor Author

@mishaschwartz mishaschwartz Feb 6, 2024

Choose a reason for hiding this comment

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

Seems to me that available ones are sufficient to handle the use case

Unless I'm missing some other way of using these handlers, I think they cover a different use case (see: #597 (comment))

I would rather have a SessionAuthHandler

Actually that would be really easy. We could do something like:

class SessionAuthHandler(AuthBase):
    def __init__(self, session):
        self.session = session

    def __call__(self, r):
        r.prepare_cookies(self.session.cookies)
        return r

I actually like this a lot better than my initial solution... if you're ok with it as well I can update this PR with the new handler

@mishaschwartz
Copy link
Contributor Author

Why not use the MagpieAuth object directly?
Alternatively, could also use ... [CookieAuthHandler]

Both of these make a request to a URL to obtain a token/cookie. The session strategy is for the use-case where you already have the session cookies. It's the same use-case that motivated this change (bird-house/birdhouse-deploy#407)

@github-actions github-actions bot added ci/doc Issue related to documentation of the package feature/cli Issues or features related to CLI operations. labels Feb 6, 2024
@mishaschwartz mishaschwartz deleted the session-authentication branch February 15, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/doc Issue related to documentation of the package feature/cli Issues or features related to CLI operations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants