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

Define a permission store (closes #384) #390

Merged
merged 20 commits into from
Dec 9, 2022

Conversation

johannhof
Copy link
Member

@johannhof johannhof commented Nov 11, 2022

This is a draft attempt at defining a permission store with a couple of questions still to be figured out, some of which are also marked as issues inline. Some high-level thoughts:

  • I've so far declared a single global permission store to be maintained by UAs, is that what we were shooting for?
  • We should still allow for UAs to have more flexibility in storing smaller-scoped or shorter-lived permissions, e.g. for a single tab. It feels like this needs to be a non-normative note as attempting to specify that might go against the idea that it can be flexible.
  • This ignores a lot of details around how we need to cross the process boundary to access permissions, though I've tried to make setting (and removing) permissions an async step.
  • As discussed with Anne before, there's the question of whether individual permissions should define their own key generation algorithms or whether they should get if-statements in the central key generation algorithm.

Requesting an early look/advice from @annevk and @jyasskin :)


Preview | Diff

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks @johannhof for this draft. The general shape of this looks right, but there's a lot of details that need to be written out. I left a bunch of inline comments that I hope are helpful.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@johannhof
Copy link
Member Author

@annevk I updated this PR (and privacycg/storage-access#138 which shows the usage) to allow for features overriding permission key algorithms without monkey-patching. Feel free to take another look and I'm happy to chat about it :)

Copy link
Member Author

@johannhof johannhof left a comment

Choose a reason for hiding this comment

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

@annevk and I went through this draft again today and discussed some improvements, which I noted down in the review here. Anne also filed a couple of bugs where we found issues that weren't caused by this patch. I'll finish up the remaining points in this review and then I think we're ready to move out of WIP state and up to general review (and merge, hopefully).

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Member

Should also eventually deal with #348 (i.e., with the automation part)

@marcoscaceres marcoscaceres mentioned this pull request Nov 29, 2022
1 task
@johannhof johannhof changed the title WIP: Define a permission store (closes #384) Define a permission store (closes #384) Nov 29, 2022
@johannhof
Copy link
Member Author

Ok I think this is ready for prime-time review and (hopefully) merge. The build is failing because for some reason [=origin=] isn't a valid reference anymore, happy to get pointers to that.

@johannhof
Copy link
Member Author

And I just want to acknowledge that there is definitely some follow-up work given some of the issues e.g. around lifetimes vs. keys and automation (as Marcos mentioned), but I think this is a pretty good scope for a single PR right now and can stand pretty well on its own without introducing too much inconsistency until we get to those follow-ups.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
johannhof and others added 3 commits December 1, 2022 23:43
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
@johannhof
Copy link
Member Author

@marcoscaceres @miketaylr I'd love to get your review whenever you have a few minutes. I know a lot happened here but the overall diff shouldn't be that much to review. :)

Copy link
Member

@miketaylr miketaylr left a comment

Choose a reason for hiding this comment

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

LGTM - thank you!

index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
Co-authored-by: Mike Taylor <[email protected]>
@marcoscaceres
Copy link
Member

Thanks @johannhof! I'm going to review it today.

index.html Outdated Show resolved Hide resolved
Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

lgtm... just a nit.

@marcoscaceres
Copy link
Member

@johannhof, feel free to merge is you are done making any changes.

Co-authored-by: Marcos Cáceres <[email protected]>
@johannhof
Copy link
Member Author

Thank you, @marcoscaceres! I think we're good to merge!

@johannhof johannhof merged commit f3b9273 into w3c:main Dec 9, 2022
@johannhof johannhof deleted the permission-store branch December 9, 2022 09:12
github-actions bot added a commit that referenced this pull request Dec 9, 2022
SHA: f3b9273
Reason: push, by johannhof

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@johannhof
Copy link
Member Author

Looks like the tidy job for this failed on main, will there be an automated PR for that or should I push a manual fix to main?

@miketaylr
Copy link
Member

A bot usually handles that (#406). All good!

johannhof added a commit to privacycg/storage-access that referenced this pull request Jan 5, 2023
This is building on top of w3c/permissions#390 to integrate SAA with permissions. It's deleting a lot of old manual state management but doesn't get rid of the (global) storage access map altogether, since that is done in #141.

Co-authored-by: Anne van Kesteren <[email protected]>
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.

5 participants