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
Explain operation and issuers #218
Explain operation and issuers #218
Conversation
Update Modifications to request section to include how operation and issuers fields are used.
spec.bs
Outdated
1. If |init|["{{RequestInit/privateToken}}"]["{{PrivateToken/issuers}}"] is [=list/empty=], then throw {{TypeError}}. | ||
1. [=list/For each=] |issuer| of |init|["{{RequestInit/privateToken}}"]["{{PrivateToken/issuers}}"]: | ||
1. If |issuer| is not HTTP family, then throw {{TypeError}}. | ||
1. If |issuer| is not trustworthy, then throw {{TypeError}}. |
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.
What does trustworthy mean in that context? That it's HTTPS? If it's just that you could remove this step and replace my above suggestion with:
If |issuerURL|'s [=url/scheme=] is not `"https"`, then throw {{TypeError}}.
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.
spec.bs
Outdated
Add the following steps to the <code><a constructor lt="Request()">new Request (<var ignore>input</var>, |init|)</a></code> constructor, before step 28 ("<code>Set [=this=]'s [=Request/request=] to |request|</code>"): | ||
|
||
1. If |init|["{{RequestInit/privateToken}}"] [=map/exists=]: | ||
1. Set <var ignore>request</var>'s [=request/operation=]</a> to |init|["{{RequestInit/privateToken}}"]["{{PrivateToken/operation}}"]. |
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.
You're not using the operation right now. The operation should be used in e.g. Modifications to http-network-or-cache fetch
to determine which algorithm to run, if any.
I'm slightly confused that there are 3 operation types but only 2 algorithms to run in the fetch steps, but I'm probably missing something here.
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.
I will check the fetch steps and get back to you.
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.
Added algorithm for sending redemption records. Added a new step to Modifications to http-network-or-cache fetch
.
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.
Ah, great! But you're still not using the operation as mentioned below :)
Co-authored-by: Johann Hofmann <mail@johann-hofmann.com>
Co-authored-by: Johann Hofmann <mail@johann-hofmann.com>
Co-authored-by: Johann Hofmann <mail@johann-hofmann.com>
Co-authored-by: Johann Hofmann <mail@johann-hofmann.com>
Add new algorithm for sending redemption record. Add this new algorithm to fetch modifications list.
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 for adding the steps to send redemption records! Had a few more comments but I like the shape that this is taking!
spec.bs
Outdated
Add the following steps to the <code><a constructor lt="Request()">new Request (<var ignore>input</var>, |init|)</a></code> constructor, before step 28 ("<code>Set [=this=]'s [=Request/request=] to |request|</code>"): | ||
|
||
1. If |init|["{{RequestInit/privateToken}}"] [=map/exists=]: | ||
1. Set <var ignore>request</var>'s [=request/operation=]</a> to |init|["{{RequestInit/privateToken}}"]["{{PrivateToken/operation}}"]. |
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.
Ah, great! But you're still not using the operation as mentioned below :)
1. [=map/For each=] |issuer| -> |record| of |records_per_issuer|: | ||
1. Let |serializedIssuer| be result of serializing |issuer|. | ||
1. Let |serializedRecord| be result of serializing |record|. | ||
1. [=list/Append=] |serializedIssuer| and |serializedRecord| pairs to |headerItems|. |
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.
I'm not an expert on structured headers but instead of "pairs" I think you need to construct an inner list here?
1. If |record| is null, then [=iteration/continue=]. | ||
1. [=map/Set=] |records_per_issuer|[|issuer|] to |record|. | ||
1. If |records_per_issuer| is empty, then abort these steps. | ||
1. Let |headerItems| be a structured headers list [[RFC8941]]. |
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.
Can you link to https://www.rfc-editor.org/rfc/rfc8941.html#name-lists please?
1. Let |serializedIssuer| be result of serializing |issuer|. | ||
1. Let |serializedRecord| be result of serializing |record|. | ||
1. [=list/Append=] |serializedIssuer| and |serializedRecord| pairs to |headerItems|. | ||
1. Let |serializedHeaderItems| be result of serializing |headerItems|. |
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.
I might be wrong but that seems unnecessary to me.
1. [=list/Append=] |serializedIssuer| and |serializedRecord| pairs to |headerItems|. | ||
1. Let |serializedHeaderItems| be result of serializing |headerItems|. | ||
1. If |serializedHeaderItems| is null, then abort these steps. | ||
1. Set <a http-header>`Sec-Redemption-Record`</a> to |serializedHeaderItems|. |
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.
Please use set a structured field value
as in https://wicg.github.io/trust-token-api/#append-private-state-token-issue-request-headers
More descriptive names for request associated fields in modifications to request section. Update modifications to fetch section.
Fix language in send redemption record operation. Co-authored-by: Johann Hofmann <mail@johann-hofmann.com>
I think only remaining comment to resolve is structured headers. I plan to merge this and create new issue/PR for the structured headers. |
Created #238 to capture the work on structured fields. I will merge and close this one. |
SHA: e59cbfe Reason: push, by aykutbulut Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Update Modifications to request section to include how operation and issuers fields are used.
Fixes #151
Preview | Diff