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

Cookie retrieval #3068

Merged
merged 18 commits into from
Jan 8, 2025
Merged

Cookie retrieval #3068

merged 18 commits into from
Jan 8, 2025

Conversation

kloknibor
Copy link
Contributor

Added support of retrieval of Cookies from webview components

Currently, cookies can only be retrieved with javascript. However, HTTP-only cookies can not be retrieved, and a native implementation with Python is easier.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@kloknibor
Copy link
Contributor Author

I might need some guidance on how to implement the testbed, since cookies are dynamic. Should I :

  • just check if there is any result?
  • Find/create a page with static cookies?

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This is a nice first pass - the high level structure all looks pretty good; I've flagged a couple of minor API design and testing issues inline.

Regarding the testbed: If at all possible, we'd prefer to avoid a dependency on external servers. My suggestion would be to use a javascript blob to programatically set some cookie values, and then retrieve then with the new API.

If that proves prohibitively complex, an alternative approach might be to use a "known reliable" URL to check real world behavior. Historically, we've used github.com for this purpose; the testing risk is that we're not in control of Github's cookie usage, so it will be difficult to construct a test that won't break for reasons outside our control.

One other detail that will come out of adding the testbed test is that the testbed test will fail on Android and GTK - platforms that have a WebView, but won't have an implementation of this API. You'll need to provide a stub implementation on each of those platforms that uses factory.not_implemented().

cocoa/src/toga_cocoa/widgets/webview.py Outdated Show resolved Hide resolved
dummy/src/toga_dummy/widgets/webview.py Show resolved Hide resolved
core/tests/widgets/test_webview.py Outdated Show resolved Hide resolved
changes/3068.feature.rst Outdated Show resolved Hide resolved
core/src/toga/widgets/webview.py Outdated Show resolved Hide resolved
making call more pythonic as suggested
@kloknibor kloknibor requested a review from freakboy3742 January 7, 2025 09:06
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I've flagged a couple of things inline, but they're all either fairly minor naming convention or formatting issues, or structural issues with testing that are a bit harder to explain - so I've pushed an update that includes those fixes.

Thanks for the PR - This will make a nice addition to Toga's API.

def cookies(self, on_result=None):
# Create the result object
result = CookiesResult(on_result=on_result)
result.set_result(None)
Copy link
Member

Choose a reason for hiding this comment

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

Good catch on returning a "real" result here; however, to minimise the need for workaround code, it might be helpful to return an empty CookieJar, rather than None. If nothing else, it will remove the need for an | None extension to any type annotations :-)

Comment on lines 53 to 54
print("kaas")
print(cookies)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like some stray debug.

Suggested change
print("kaas")
print(cookies)

@@ -135,6 +139,23 @@ def set_content(self, root_url: str, content: str) -> None:
"""
self._impl.set_content(root_url, content)

def cookies(
Copy link
Member

Choose a reason for hiding this comment

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

We're generally deprecating the use of the on_result "callback" version of asynchronous APIs; so this could be turned into an effectively async property.

**This is an asynchronous method**. There is no guarantee that the function
has finished evaluating when this method returns. The object returned by this
method can be awaited to obtain the value of the expression.
An CookieJar object will be returned
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
An CookieJar object will be returned

Return values should be documented explicitly - see below.


**Note:** This is not yet currently supported on Android or Linux.

:param on_result: A callback function to process the cookies once retrieved.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:param on_result: A callback function to process the cookies once retrieved.
:param on_result: A callback function to process the cookies once retrieved.
:returns: A CookieJar object with all the cookies currently defined on the page.


:param on_result: A callback function to process the cookies once retrieved.
"""
return self._impl.cookies(on_result=on_result)
Copy link
Member

Choose a reason for hiding this comment

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

For consistency with other backend APIs, this should remain a get_cookies() method. It's not a public API, it's an internal one.

result = widget.cookies()

# Get the cookie jar from the future
cookie_jar = await result.future # Await the future to get the CookieJar
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to await result directly - there's no need to dig into the future.

# Get the cookie jar from the future
cookie_jar = await result.future # Await the future to get the CookieJar

# Validate the cookies in the CookieJar
Copy link
Member

Choose a reason for hiding this comment

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

This should also validate that the result is a cookiejar, not just that it is iterable.


cookie_jar = await result.future # Await the future to get the CookieJar

if toga.platform.current_platform not in {"windows", "iOS", "macOS"}:
Copy link
Member

Choose a reason for hiding this comment

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

We try not to reference specific platforms in the individual tests - instead, we put methods and properties on the test probe that raise a skip if the platform doesn't implement the feature.

"""Cookies can be retrieved asynchronously."""
# A page must be loaded to set cookies
await wait_for(
widget.load_url("https://example.com/"),
Copy link
Member

Choose a reason for hiding this comment

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

We lean on github.com as a "known URL" because it should be a local lookup when running on GitHub Actions (also - at time of writing, example.com appears to be down for me, which isn't a good sign...)

@freakboy3742 freakboy3742 merged commit 738f444 into beeware:main Jan 8, 2025
41 checks passed
@kloknibor
Copy link
Contributor Author

kloknibor commented Jan 8, 2025

Good catches! Thanks for the support both here and at discord, it's very much appreciated :-) This was a fun project

@mhsmith
Copy link
Member

mhsmith commented Jan 12, 2025

One of these new tests has just failed on the main branch on macOS.

@freakboy3742
Copy link
Member

Yeah - there were multiple failures of this kind in the weekly dependabot updates. I'll need to investigate options for a more robust test.

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.

3 participants