<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>