-
Notifications
You must be signed in to change notification settings - Fork 51
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
Conversation
There was a problem hiding this 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.
@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 :) |
There was a problem hiding this 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).
Should also eventually deal with #348 (i.e., with the automation part) |
Co-authored-by: Anne van Kesteren <[email protected]>
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 <[email protected]>
Co-authored-by: Jeffrey Yasskin <[email protected]>
Co-authored-by: Jeffrey Yasskin <[email protected]>
Co-authored-by: Jeffrey Yasskin <[email protected]>
Co-authored-by: Anne van Kesteren <[email protected]>
@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. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thank you!
Co-authored-by: Mike Taylor <[email protected]>
Thanks @johannhof! I'm going to review it today. |
There was a problem hiding this 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.
@johannhof, feel free to merge is you are done making any changes. |
Co-authored-by: Marcos Cáceres <[email protected]>
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 <[email protected]>
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