This was filed downstream at https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=219133

*** Bug 156887 has been marked as a duplicate of this bug. ***

Here is a semi-working patch for authentication. 
It successfully asks for a password and populates the printer list
on  a password-protected cups server. What does not work yet is
the GET-request to get the ppd. 

Created attachment 90867 [details] [review]
patch

+ response = gtk_dialog_run (GTK_DIALOG (dialog));

Is it possible to make that dialogue run async? Running a recursive mainloops is a problem for epiphany, since you have to do preparations before (push a NULL JS context on the stack); which is obviously impossible if the gtk_dialog_run is inside the library, out of the app's control.

Oh and async auth callbacks are also preferable for integration with gnome-keyring. And also we'll need a way to hook up to the auth from outside the print dialogue.

Yeah, this is not a finished patch - as I indicated, it does not quite work yet. 
And the the gtk_dialog_run will certainly go away in the final version. 

i don't think this patch will work with kerberos/negotiate. i've just written a quick hack to do this for gtk+-2.10.14 because i needed it urgently (and as a quick hack it doesn't support basic authentication, which i'm not using). could the "final solution" support negotiate too?

i'll attach my patch for reference, but it's pretty dirty. it'll only work with cups-1.3 or later.

Created attachment 107308 [details] [review]
a quick patch to add cups negotiation support

Created attachment 114314 [details] [review]
Pause state and Rejecting Jobs state handling patch.

Hi,
this patch handle situations when the printer is paused or is not accepting jobs. In the case of paused printer the row with the printer is greyed out and the print button is insensitive.
If the printer is rejecting jobs and the print button is pressed, the error message is displayed.
Rejected users handling is not implemented in this patch. The printer to which the user is not allowed to send jobs is not listed by cups.
Authentication is not implemented yet as well.

  Marek

I think you have it backwards wrt to the graying out...

A paused printer may still accept jobs and queue them, it is just not printing them. So, you probably want to gray out the !accepting-jobs printers, but let the user still queue jobs on paused printers, probably just indicate that the printer is paused, somehow

I'll rework it.
I implemented it according to opening comment.

I would like to indicate paused state in states message of the printer.
But there is a problem with printer-state-message attribute for paused printer state. Some printers show "Paused" in printer-state-message attribute, but some doesn't. It is little worse, because this message should be generated in any of the natural languages identified by the Printer's "generated-natural-language-supported" attribute. So I cannot find out whether it is already present in "printer-state-message" attribute (except the state when the attribute is empty string).
  - I'll probably do nothing in the case of paused printer, because it is indicated by printer-state-message in most cases
  - I can check length of printer-state-message for 0

It would be nice to have it shown also graphically by an icon in the first column. Is it acceptable to bring in new icons?

Rejecting printer will be greyed out and the print button will be insensitive.
So there is no need to show an error dialog now.

  Marek

I think the icon might be a nice idea. I can ask mike langlie to draw us one.

Hi Matthias, it would be cool.

  Thank you

    Marek

Created attachment 114321 [details] [review]
Reworked patch.

Reworked patch for points 1 and 2 from opening comment.

  Marek

Looks good to me now.

We can add the icon later when Mike has them done.

Created attachment 114528 [details]
a patch to handle paused or rejecting printer

The previous patch with "gtk-print-paused" icons.

  Marek

Hi,
the icons were designed by Mike Langlie.

  Marek

Thanks for the updated patch. It needs some more work:

- Please add boolean properties "paused" and "accepting-jobs" to GtkPrinter.

- The Since: tags should name the next stable release, ie: Since: 2.14

- Please add a little explanation of what "paused" means to the docs of 
  gtk_printer_is_paused(). Something like: A paused printer still accepts 
  jobs, but it is not printing them.

- Please add the new functions to the right section in gtk/gtk.symbols 
  and in docs/reference/gtk/gtk-sections.txt


Note that I've already committed this bugfix for the other icons:
@@ -1108,7 +1109,11 @@
        stock-icons/24/gtk-paste.png                    \
        stock-icons/24/gtk-preferences.png              \
        stock-icons/24/gtk-print.png                    \
+       stock-icons/24/gtk-print-error.png              \
+       stock-icons/24/gtk-print-paused.png             \
        stock-icons/24/gtk-print-preview.png            \
+       stock-icons/24/gtk-print-report.png             \
+       stock-icons/24/gtk-print-warning.png            \
        stock-icons/24/gtk-properties.png               \
        stock-icons/24/gtk-quit.png                     \
        stock-icons/24/gtk-redo-ltr.png                 \

Created attachment 114605 [details]
Reworked patch.

Hi,
the previous patch reworked according to the comment #19.

  Regards

    Marek

Thanks.

One thing I forgot to mention is that we also need to register the new stock icon in gtkiconfactory.c. But I'll handle that when committing.

I've committed this patch, thanks. Leaving the bug open to handle authentication, some day.

2008-07-15  Matthias Clasen  <mclasen@redhat.com>

        Bug 384940 – handle rejecting jobs and authentication meaningfully

        Patch by Marek Kasik, icons by Mike Langlie:
        * gtk/gtk.symbols:
        * gtk/gtkprintbackend.h:
        * gtk/gtkprinter.[hc]: Add new paused and accepting-jobs properties
        and getters/setters.

        * gtk/gtkstock.h:
        * gtk/gtkiconfactory.c:
        * gtk/Makefile.am:
        * gtk/stock-icons/{16,24}/gtk-print-paused.{png,svg}: New icon.

        * modules/printbackends/cups/gtkprintbackendcups.c:
        * gtk/gtkprintunixdialog.c: Handle paused and job-rejecting
        printers.

How do you envision authentication being handled?

*** Bug 566522 has been marked as a duplicate of this bug. ***

*** Bug 553196 has been marked as a duplicate of this bug. ***

Created attachment 130196 [details] [review]
an authentication patch

Hi,
this patch add authentication of users against CUPS server. It works for Basic, BasicDigest, Digest, Negotiate, PeerCred and None authentication.
It is a modified version of Matthias' patch.
The password dialog was moved to the gtkprintbackend.c to be able to call it even if GtkPrintUnixDialog doesn't already exist (authentication of Print-Job).
It doesn't contain Samba support.

  Marek

Some initial comments:

- Moving the dialog code to GtkPrintbackend has the unfortunate side effect that we don't set the parent anymore. I have no immediate idea what to do about it.


+void 
+fallback_set_password (GtkPrintBackend *backend,
+                       const gchar     *hostname,
+                       const gchar     *username,
+                       const gchar     *password)

This should be static. Actually, i'm not sure why I had that in my patch. It would be more customary to do

 if (GTK_PRINT_BACKEND_GET_CLASS (backend)->set_password)
   GTK_PRINT_BACKEND_GET_CLASS (backend)->set_password (...)


+  text = g_strdup_printf ( _("Please enter the password for %s on %s"), username, hostname);

I think this needs more context. I guess we may just copy the dialog that Tim uses:
http://cyberelk.net/tim/wp-content/uploads/2009/02/authentication-dialog.png

Of course, that means we also need to support changing the user. Is that feasible ?


static char *cups_password;
+
+static const char *
+passwordCB (const char *prompt)
+{
+  char *pwd = cups_password;
+  cups_password = NULL;
+  return pwd;
+}

Not sure I understand my own code anymore here. Who is reponsible for freeing cups_password ?


+ httpGetHostname (dispatch->request->http, dispatch_hostname, sizeof (dispatch_hostname));
+      if (dispatch_hostname[0] == '/' ||
+          strcmp (dispatch_hostname, "127.0.0.1") == 0 ||
+          strcmp (dispatch_hostname, "[::1]") == 0)
+        strcpy (dispatch_hostname, "localhost");


This should perhaps be singled out as a helper function, since it occurs more than once.


+  gtk_widget_show_all (dialog);
+  gtk_widget_show (dialog);

show_all is enough.

Created attachment 130884 [details] [review]
modified patch

Hi,
I removed the fallback_set_password().
The authentication dialog allows user to change username now. It also changes its prompt according to the authenticated action.
The freeing of "static char *cups_password;" handles the patch now (CUPS is not responsible for its freeing).
The patch contains function is_adress_local() to handle the locality of given address.
I removed the call of gtk_widget_show().

Tim wrote me that it would be good to use gnome-keyring to store authentication informations. What do you think about this?

  Marek

As an end user I would love to have gnome-keyring support. Retyping passwords on every print job could become tedious.

static const char *
+passwordCB (const char *prompt)
+{
+  char *pwd = cups_password;
+  cups_password = NULL;
+
+  cupsSetUser (cups_username);
+
+  return pwd;
+}

I'm a little concerned about this, since the cups user seems to be global ? 
What if there are multiple print jobs in flight ? Not sure if we can do anything about it...might be worth checking with twaugh.

> The freeing of "static char *cups_password;" handles the patch now (CUPS is not
> responsible for its freeing).

I didn't notice that. Where is it happening ?


> Tim wrote me that it would be good to use gnome-keyring to store authentication informations. What do you think about this?

Yes, that would probably be good. I think we should try to do that in a second round after landing the initial patch, though, since there are some complications:

- We probably need to make that conditional on some --enable-gnome-keyring configure switch, and even then, we don't want libgtk itself to depend on gnome-keyring, so the dependency should be confined to the print backend.

- We need to determine in what form to store these passwords. I think it would be annoying if each application asks for the password again, so they should probably be stored session wide and shared by all apps, and also by system-config-printer. Does s-c-p already use gnome-keyring ?

> I'm a little concerned about this, since the cups user seems to be global ? 
> What if there are multiple print jobs in flight ? Not sure if we can do
> anything about it...might be worth checking with twaugh.

I wrote this according to CUPS documentation (http://www.cups.org/doc-1.4/api-cups.html#PASSWORDS_AND_AUTHENTICATION).


> I didn't notice that. Where is it happening ?

The password is freed here:
(the cups_password is pointer to request->password)
+              if (request->password != NULL) 
+                { 
+                  memset (request->password, 0, strlen (request->password)); 
+                  g_free (request->password); 
+                  request->password = NULL; 
+                } 
+ 
+              request->password_state = GTK_CUPS_PASSWORD_APPLIED; 
+            } 


> Yes, that would probably be good. I think we should try to do that in a second
> round after landing the initial patch, though, since there are some
> complications:

OK, lets leave the gnome-keyring support for another round.

  Marek

I see that you tried to improve the contents of the password dialog a bit. It would be great if we could get something like twaughs dialog has:

"Authentication is required to print document 'My greatest successes' on printer HP foobar 1"

Can we do that ?

> The password is freed here:
> (the cups_password is pointer to request->password)
> +              if (request->password != NULL) 
> +                { 
> +                  memset (request->password, 0, strlen (request->password)); 
> +                  g_free (request->password); 
> +                  request->password = NULL; 
> +                } 
> + 
> +              request->password_state = GTK_CUPS_PASSWORD_APPLIED; 
> +            } 

But right before that code you have

+              if (cups_password != NULL)
+                return;

so if cups_password == request->password, I don't see how the g_free (request->password) can ever have an effect. 


+      g_signal_emit_by_name (dispatch->backend, "request-password", 
+                             hostname, username, prompt);

I think you need to free prompt after this.


+  text = g_strdup_printf (prompt);

This looks wrong.


              cups_password = g_strdup("");

Missing space before (.


> It works for Basic, BasicDigest, Digest, Negotiate, PeerCred and None 
> authentication

Have you tested all these ?

Created attachment 131346 [details] [review]
modified patch

> I see that you tried to improve the contents of the password dialog a bit. It
> would be great if we could get something like twaughs dialog has:
> 
> "Authentication is required to print document 'My greatest successes' on
> printer HP foobar 1"
>
> Can we do that ?

Added (we get job's name and printer's name from ipp_request_t by new function gtk_cups_request_ipp_get_string() ).

> > The password is freed here:
> > (the cups_password is pointer to request->password)
It should be: 
(the cups_password points to the same place as the request->password)

> > +              if (request->password != NULL) 
> > +                { 
> > +                  memset (request->password, 0, strlen (request->password)); 
> > +                  g_free (request->password); 
> > +                  request->password = NULL; 
> > +                } 
> > + 
> > +              request->password_state = GTK_CUPS_PASSWORD_APPLIED; 
> > +            } 

> But right before that code you have

> +              if (cups_password != NULL)
> +                return;

> so if cups_password == request->password, I don't see how the g_free
> (request->password) can ever have an effect. 

cups_password and request->password points to same place, but cups_password is set to NULL (not freed) when passwordCB is called.
So, request->password still contains the pointer.


> +      g_signal_emit_by_name (dispatch->backend, "request-password", 
> +                             hostname, username, prompt);
> 
> I think you need to free prompt after this.

Fixed.

> +  text = g_strdup_printf (prompt);
> 
> This looks wrong.

Fixed.

>               cups_password = g_strdup("");
> 
> Missing space before (.

Fixed.


> > It works for Basic, BasicDigest, Digest, Negotiate, PeerCred and None 
> > authentication
> 
> Have you tested all these ?

Yes, I did. Is anything wrong?

  Thank you for your comment

    Marek

+  text = g_strdup (prompt);

Why are you doing that ? I think you could just use the prompt directly in the g_markup_printf_escaped call.


> > Have you tested all these ?
>
> Yes, I did. Is anything wrong?

No, just that I am not prepared to test all these combinations myself and I wanted to make sure that we actually know that they all work.


Apart from the one thing I mentioned, the patch looks good to go. Needs to wait until we branch, though.

> +  text = g_strdup (prompt);
> 
> Why are you doing that ? I think you could just use the prompt directly in the
> g_markup_printf_escaped call.

You are right. I'll remove it before commit.

  Marek

One more question, looking at bug 553690. Does this patch support gssapi, too ?

Hi Matthias,
I tested the patch with Kerberos. It worked for me. I'll test it again before commit if you want.

  Marek

No, its fine if you tested it. I just wasn't clear whether 553690 is a duplicate of this or not. Sounds like it is...

Please commit to master.