Define a permission store (closes #384)#390
Conversation
annevk
left a comment
There was a problem hiding this comment.
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.
|
@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 :) |
johannhof
left a comment
There was a problem hiding this comment.
@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).
|
Should also eventually deal with #348 (i.e., with the automation part) |
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
189b999 to
1dfe4fd
Compare
|
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. |
|
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. |
Co-authored-by: Jeffrey Yasskin <jyasskin@gmail.com>
Co-authored-by: Jeffrey Yasskin <jyasskin@gmail.com>
Co-authored-by: Jeffrey Yasskin <jyasskin@gmail.com>
Co-authored-by: Jeffrey Yasskin <jyasskin@gmail.com>
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
|
@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. :) |
Co-authored-by: Mike Taylor <miketaylr@google.com>
|
Thanks @johannhof! I'm going to review it today. |
|
@johannhof, feel free to merge is you are done making any changes. |
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
|
Thank you, @marcoscaceres! I think we're good to merge! |
|
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? |
|
A bot usually handles that (#406). All good! |
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 <annevk@annevk.nl>
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:
Requesting an early look/advice from @annevk and @jyasskin :)
Preview | Diff