<html>
<head>
<base href="https://bugs.freedesktop.org/">
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Implement digital signature support (glib frontend)"
href="https://bugs.freedesktop.org/show_bug.cgi?id=94376#c2">Comment # 2</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Implement digital signature support (glib frontend)"
href="https://bugs.freedesktop.org/show_bug.cgi?id=94376">bug 94376</a>
from <span class="vcard"><a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=124622" name="attach_124622" title="Expose signature verification in the glib frontend">attachment 124622</a> <a href="attachment.cgi?id=124622&action=edit" title="Expose signature verification in the glib frontend">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=94376&attachment=124622'>[review]</a>
Expose signature verification in the glib frontend
Review of <span class=""><a href="attachment.cgi?id=124622" name="attach_124622" title="Expose signature verification in the glib frontend">attachment 124622</a> <a href="attachment.cgi?id=124622&action=edit" title="Expose signature verification in the glib frontend">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=94376&attachment=124622'>[review]</a>:
-----------------------------------------------------------------
Thanks for the patch and sorry for the delay to review it. I haven't followed
the core implementation, so I'm not sure how this API is expected to be used by
applications. I guess the general idea is that the document might contain one
or more signatures (defined as form fields), and applications can verify the
document by validating every form field, right?
::: poppler-tmp/poppler-0.45.0/glib/poppler-document.cc
@@ +1105,4 @@
<span class="quote">> }
>
> /**
> + * poppler_document_count_signatures:</span >
In glib API we normally use n_signatures instead of count
@@ +1119,5 @@
<span class="quote">> +poppler_document_count_signatures(PopplerDocument *document)
> +{
> + g_return_val_if_fail (POPPLER_IS_DOCUMENT (document), 0);
> + return document->doc->getSignatureWidgets().size();
> +}</span >
What's the point of providing the number of signatures, how is the user
expected to use this? Just to know whether the document needs to be verified?
@@ +1478,5 @@
<span class="quote">> FALSE,
> G_PARAM_READABLE));
> + /**
> + * PopplerDocument::signed:
> + * Whether the document is signed.</span >
This doesn't match the property definition. I'm not sure we need a property for
this, though.
::: poppler-tmp/poppler-0.45.0/glib/poppler-form-field.cc
@@ +703,5 @@
<span class="quote">> +
> + new_info = g_new0(PopplerSignatureInfo, 1);
> + new_info->sig_status = sig_info->sig_status;
> + new_info->cert_status = sig_info->cert_status;
> + new_info->signer_name = sig_info->signer_name;</span >
This is owned by the other PopplerSignatureInfo, if you don't duplicate the
string, freeing one structure would leave the other one with an invalid
pointer.
@@ +704,5 @@
<span class="quote">> + new_info = g_new0(PopplerSignatureInfo, 1);
> + new_info->sig_status = sig_info->sig_status;
> + new_info->cert_status = sig_info->cert_status;
> + new_info->signer_name = sig_info->signer_name;
> + new_info->signing_time = sig_info->signing_time;</span >
You can do *new_info = *sig_info and then copy manually only the fields needed,
the signer name in this case.
@@ +712,5 @@
<span class="quote">> +
> +void
> +poppler_signature_info_free (PopplerSignatureInfo *siginfo)
> +{
> + g_free(siginfo);</span >
And here you should also free the signer name
@@ +716,5 @@
<span class="quote">> + g_free(siginfo);
> +}
> +
> +PopplerSignatureInfo *
> +poppler_form_field_signature_validate (PopplerFormField *sigField, gboolean doVerifyCert, gboolean forceRevalidation)</span >
This method also need to be documented. What do the parameters mean? I prefer
to use flags oinstead of boolean parameters if possible.
@@ +725,5 @@
<span class="quote">> + FormFieldSignature * sig_field = static_cast<FormFieldSignature*>(sigField->widget->getField());
> + SignatureInfo * sig_info = sig_field->validateSignature(doVerifyCert, forceRevalidation);
> +
> + PopplerSignatureInfo * poppler_sig_info;
> + poppler_sig_info = g_new0(PopplerSignatureInfo, 1);</span >
Maybe this could be an out parameter, so that it can be stack allocated by the
caller. And the method could return the validation result.
@@ +772,5 @@
<span class="quote">> + case CERTIFICATE_GENERIC_ERROR:
> + poppler_sig_info->cert_status = POPPLER_CERTIFICATE_GENERIC_ERROR;
> + break;
> + case CERTIFICATE_NOT_VERIFIED:
> + poppler_sig_info->cert_status = POPPLER_CERTIFICATE_NOT_VERIFIED;</span >
So we are validating two things in the same function? Are really sig and cert
status part of the signature information?
@@ +776,5 @@
<span class="quote">> + poppler_sig_info->cert_status = POPPLER_CERTIFICATE_NOT_VERIFIED;
> + break;
> + }
> +
> + poppler_sig_info->signer_name = sig_info->getSignerName();</span >
The return value is owned by SignatureInfo, we should copy it, and
SignatureInfo should return a const char* instead.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>