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

chore(weave): Uses anon cookie, if no user is authed #1604

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jwlee64
Copy link
Contributor

@jwlee64 jwlee64 commented May 6, 2024

This pr does two things
adds a function to create a dummy cookie, that we use in the app to create a cookie for anon users
adds support that if a user is anon (no userid) that we use said cookie as their cache key

@circle-job-mirror
Copy link

circle-job-mirror bot commented May 6, 2024

@circle-job-mirror
Copy link

@jwlee64 jwlee64 marked this pull request as ready for review May 9, 2024 15:59
@jwlee64 jwlee64 requested review from a team as code owners May 9, 2024 15:59
@@ -157,3 +157,14 @@ function getFirebaseCookiesObject(): Struct<string> {
return {};
}
}

export const generateCookieString = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not put this in the core pr where it is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like all the cookie helpers were in this file

return ctx.user_id
if ctx.user_id:
return ctx.user_id
if ctx.cookies and ctx.cookies["anon_wandb"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

i think it would be cleaner if the weave cache logic did not have to know about the specific cookies / cookie name. This feels like we are coupling network implementation details with cache details. Is there a way that the middleware layer could store the anon_wandb cookie value in the context in such a way that it is clearly an anonymous session id?

Note: these cookies should be considered "black box" inside the the weave application and are just forwarded to gorilla.

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.

None yet

2 participants