GNOME Bugzilla – Bug 746455
Cancel SoupMessage processing when should not authenticate
Last modified: 2015-03-20 15:18:40 UTC
Created attachment 299817 [details] [review] Cancel SoupMessage processing when should not authenticate Cancel the message processing if soup_auth_authenticate should not be called because of NULL parameters. It can help determining the cause of authentication problem and it also silences runtime warnings in remote-viewer like: libsoup-CRITICAL **: soup_auth_authenticate: assertion 'password != NULL' failed
Review of attachment 299817 [details] [review]: ::: rest/rest-proxy.c @@ +239,3 @@ + if (try_auth && !rest_proxy_auth_is_paused (rest_auth)) { + if (priv->username != NULL && priv->password != NULL) + soup_auth_authenticate (soup_auth, priv->username, priv->password); Hmm I think it could sometimes be valid to try to authenticate with no username/password? Maybe introducing rest_proxy_auth_cancel() in the public API would be better?
(In reply to Christophe Fergeau from comment #1) > Review of attachment 299817 [details] [review] [review]: > > ::: rest/rest-proxy.c > @@ +239,3 @@ > + if (try_auth && !rest_proxy_auth_is_paused (rest_auth)) { > + if (priv->username != NULL && priv->password != NULL) > + soup_auth_authenticate (soup_auth, priv->username, priv->password); > > Hmm I think it could sometimes be valid to try to authenticate with no > username/password? Hm, in soup_auth_authenticate there are checks for NULL parameters, so the call has no effect: g_return_if_fail (username != NULL); g_return_if_fail (password != NULL); > Maybe introducing rest_proxy_auth_cancel() in the public API would be better? Ok, I will try this approach
(In reply to Christophe Fergeau from comment #1) > Review of attachment 299817 [details] [review] [review]: > > ::: rest/rest-proxy.c > @@ +239,3 @@ > + if (try_auth && !rest_proxy_auth_is_paused (rest_auth)) { > + if (priv->username != NULL && priv->password != NULL) > + soup_auth_authenticate (soup_auth, priv->username, priv->password); > > Hmm I think it could sometimes be valid to try to authenticate with no > username/password? Would not be the case to send down "" instead of NULL? Or at least the case to treat it down as "" instead of NULL?
(In reply to Fabiano Fidêncio from comment #3) > Would not be the case to send down "" instead of NULL? Looks like it given that libsoup catches NULL username or password with g_return_if_fail() checks.
I'm still wary of having some magic behaviour in the authentication handling, especially with the documentation saying * If these credentials fail, the signal will be * emitted again, with @retrying set to %TRUE, which will * continue until %FALSE is returned from the callback. (arguably, it does not say anything about what happens when the params are NULL). Explicit API to do that would be better I think.
Ok, I will do it that way.