-
Notifications
You must be signed in to change notification settings - Fork 99
feat: Add CAWG validation to Reader
#1370
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1370 +/- ##
==========================================
+ Coverage 78.41% 78.44% +0.02%
==========================================
Files 162 162
Lines 39536 39593 +57
==========================================
+ Hits 31001 31057 +56
- Misses 8535 8536 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #1370 will not alter performanceComparing Summary
Footnotes
|
Reader
ba322ff
to
5270494
Compare
I think I've solved the sync/async issues by leaving the existing approach in C API unchanged. I've added a new convenience function (async only) which does the combined test. That's what we were doing in the C API wrapper functions already and I've removed my changes to those functions. |
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.
Why not just say that Reader::from_stream_async() supports cawg but reader_from_stream() doesn't yet. The same would go for Reader:from_file() I don't see why we should add new cawg specific methods here.
Eventually, hopefully the sync methods can support cawg too.
Another option is that we can configure the cawg support with a setting. So if you have cawg enabled we always do the cawg call and if you don't there's no extra overhead.
Maybe there's a separate (or the same) option that allows the sync methods to block on the async call. you can can disable the setting if you don't want that behavior.
TO DO: Review c2patool usage. |
TO DO: Check in to whether verify-on-sign does CAWG identity assertion verification and if it becomes async as a result. |
…ies whether to interpret identity assertions during manifest reading Defaults to true, but there are some internal tests that need it off.
These were public but undocumented. Encouraging external clients to use the new JSON-style reporting added in this PR.
/// | ||
/// Aside from CBOR parsing, no further validation is performed. | ||
pub fn from_manifest<'a>( | ||
pub(crate) fn from_manifest<'a>( |
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.
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.
From my point of view the access changes are OK.
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 the fields of IdentityAssertion above be public, or can I have a more general constructor? I redefine it in the Node SDK, which is not a good practice.
https://github.com/contentauth/c2pa-node-v2/blob/main/src/neon_identity_assertion_builder.rs#L34
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 give the image a better name than cawgi? What is special about it, except that it has cawg data?
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! Very excited to get this in.
This is a sketch of the changes I'd like to see made to the
Reader
interface to make CAWG more directly part of the reading process from clients' point of view.