GNOME Bugzilla – Bug 756313
Set a User-Agent
Last modified: 2017-01-17 10:31:56 UTC
Nominatim’s usage policy requires a User-Agent. In case geocode-glib is used with Nominatim instances other than nominatim.gnome.org (bug #756311), it should set a User-Agent, and probably include the current GApplication name in there too, or allow the application to set its own User-Agent. http://wiki.openstreetmap.org/wiki/Nominatim_usage_policy
We probably need to make that a requirement, without a default, eg. make any call done without setting the user-agent fail. But that would need to be only for newer versions of the library...
Could be implemented as a property which emits a warning if not set, but the default is derived from the GApplication:application-id, which should satisfy the Nominatim policy.
Created attachment 318450 [details] [review] Add a default user-agent based on GApplication id Nominatim’s usage policy requires a User-Agent. They want to be able to block batch offenders that misuse the terms of service. We can set a user-agent based on GApplication (if we are one) to make sure at least trusted GNOME applications can be identified.
How about something like that? It could also be backported to pretty much 3.10?
sudo tshark -Y 'http contains "User-Agent:"' -T fields -e http.user_agent Running as user "root" and group "root". This could be dangerous. Capturing on 'enp0s3' libgdata/0.17.3 - gzip libgdata/0.17.3 - gzip org.gnome.Maps org.gnome.Maps org.gnome.Maps
Created attachment 318465 [details] [review] Add a default user-agent based on GApplication id Nominatim’s usage policy requires a User-Agent. They want to be able to block batch offenders that misuse the terms of service. We can set a user-agent based on GApplication (if we are one) to make sure at least trusted GNOME applications can be identified.
Or more user-agenty: Maps: Running as user "root" and group "root". This could be dangerous. Capturing on 'enp0s3' libgdata/0.17.3 - gzip libgdata/0.17.3 - gzip geocode-glib/3.18.0 (org.gnome.Maps) geocode-glib/3.18.0 (org.gnome.Maps) make check: Running as user "root" and group "root". This could be dangerous. Capturing on 'enp0s3' geocode-glib/3.18.0
Review of attachment 318465 [details] [review]: ::: geocode-glib/geocode-forward.c @@ +209,3 @@ + application = g_application_get_default (); + if (application) { + char *app_id = g_application_get_application_id (application); const gchar *app_id ::: geocode-glib/geocode-reverse.c @@ +95,3 @@ + application = g_application_get_default (); + if (application) { + char *app_id = g_application_get_application_id (application); const here too. @@ +101,3 @@ + user_agent = g_strdup_printf ("geocode-glib/%s", + PACKAGE_VERSION); + } Why is this block of code duplicated in this file? In fact, it could be factored out from both files into somewhere common. For example, a common SoupSession *geocode_build_soup_session (void); which builds the user agent and sets any other session options you add in future.
Review of attachment 318465 [details] [review]: ::: geocode-glib/geocode-reverse.c @@ +101,3 @@ + user_agent = g_strdup_printf ("geocode-glib/%s", + PACKAGE_VERSION); + PACKAGE_VERSION); Right you are! We even have such a common place! Will re-spin, thanks! Do you have an opinion on the user-agent syntax?
Created attachment 318470 [details] [review] Add a default user-agent based on GApplication id Nominatim’s usage policy requires a User-Agent. They want to be able to block batch offenders that misuse the terms of service. We can set a user-agent based on GApplication (if we are one) to make sure at least trusted GNOME applications can be identified.
Any ideas how to deal with non-GApplication applications tho? Have some kind of property to override user-agent? Or just override the (%s) part of the user-agent? PROP_USER_AGENT_ID ?
(In reply to Jonas Danielsson from comment #11) > Any ideas how to deal with non-GApplication applications tho? > Have some kind of property to override user-agent? Or just override the (%s) > part of the user-agent? > > PROP_USER_AGENT_ID ? My thinking is that the patch above can go in, and be back-ported far back. And maybe we can provide a way for new non-GApplication users with a property. Or do we care?
(In reply to Jonas Danielsson from comment #9) > Do you have an opinion on the user-agent syntax? What you have in comment #7 seems reasonable to me. (In reply to Jonas Danielsson from comment #11) > Any ideas how to deal with non-GApplication applications tho? > Have some kind of property to override user-agent? Or just override the (%s) > part of the user-agent? > > PROP_USER_AGENT_ID ? Yes, I think you should have PROP_USER_AGENT, to allow overriding the full user agent. This is a superset of PROP_USER_AGENT_ID.
Created attachment 318482 [details] [review] wip: Add user-agent override
Created attachment 318483 [details] [review] wip: Add user-agent override
Something like that I guess? The problem is it would be pretty much exactly the same code for geocode-forward and geocode-reverse, which is a bummer.
Review of attachment 318483 [details] [review]: ::: geocode-glib/geocode-forward.c @@ +56,3 @@ PROP_SEARCH_AREA, + PROP_BOUNDED, + PROP_USER_AGENT Add a trailing comma here and you will avoid diff noise on this line in future if you add more lines under it. @@ +212,3 @@ + * GeocodeForward:user-agent: + * + * If set to non-#NULL will override the default User Agent. You should document what the default is, that it uses GApplication, and also link to the official specification of what a User-Agent should look like. Also link to the Nominatim terms and conditions which motivate this. @@ +214,3 @@ + * If set to non-#NULL will override the default User Agent. + * + */ Missing a ‘Since: X.Y’ line. @@ +1183,3 @@ + * If set to non-#NULL then @user_agent will override the default + * User Agent that is constructed from geocode-glib version and + * optionally the applications #GApplication id. I would put this documentation in the comment for the property, and just have the setter say ‘Sets #GeocodeForward:user-agent.’. @@ +1185,3 @@ + * optionally the applications #GApplication id. + * + **/ Missing a ‘Since: X.Y’ line. You should use ‘*/’ to end gtk-doc comments, not ‘**/’. @@ +1191,3 @@ +{ + g_return_if_fail (GEOCODE_IS_FORWARD (forward)); + g_return_if_fail (forward->priv->soup_session != NULL); When is the Soup session created? If it’s possible for the user to call this method before the session is created, you should document this quirk. Or, ideally, fix it. @@ +1194,3 @@ + + g_free (forward->priv->user_agent); + forward->priv->user_agent = g_strdup (user_agent); Technically, you should do the g_strdup() before the g_free(), otherwise geocode_forward_set_user_agent (forward, geocode_forward_get_user_agent (forward)); will explode. @@ +1206,3 @@ + g_object_set (G_OBJECT (forward->priv->soup_session), + "user-agent", user_agent, + NULL); You could factor out the common g_object_set() call, and move the `g_free (agent);` out to the bottom too. Initialise `agent = NULL` if `user_agent != NULL`. @@ +1258,3 @@ + * + * Get the value of the #GeocodeForward:user-agent property. + * If the value is #NULL then the default User Agent is used. Same gtk-doc comment issues as above. The return value isn’t documented. You should mark it as (nullable). @@ +1263,3 @@ +geocode_forward_get_user_agent (GeocodeForward *forward) +{ + g_return_val_if_fail (GEOCODE_IS_FORWARD (forward), FALSE); s/FALSE/NULL/ ::: geocode-glib/geocode-glib-private.h @@ +57,3 @@ gboolean _geocode_object_is_number_after_street (void); SoupSession *_geocode_glib_build_soup_session (); +char *_geocode_glib_user_agent (); (void), otherwise this takes an undefined set of parameters. ::: geocode-glib/geocode-glib.c @@ +39,3 @@ +gchar * +_geocode_glib_user_agent () (void) @@ +55,3 @@ + +SoupSession * +_geocode_glib_build_soup_session () (void) @@ +58,3 @@ +{ + SoupSession *session; + char *user_agent = _geocode_glib_user_agent (); This is leaked at the end of the function.
Review of attachment 318483 [details] [review]: Thanks a lot Philip! It feels a bit wrong to add pretty much the exactly same code and comments to geocode-reverse.c as well (and geocode-reverse does not even have properties at the moment, so it will add boilerplate code as well). Do you or Bastian? see anyway around it? ::: geocode-glib/geocode-forward.c @@ +214,3 @@ + * If set to non-#NULL will override the default User Agent. + * + */ geocode-glib does not have Since lines... Would starting now be confusing? @@ +1191,3 @@ +{ + g_return_if_fail (GEOCODE_IS_FORWARD (forward)); + * optionally the applications #GApplication id. No, it is not, will remove this line.
Created attachment 318491 [details] [review] Add a default user-agent based on GApplication id Nominatim’s usage policy requires a User-Agent. They want to be able to block batch offenders that misuse the terms of service. We can set a user-agent based on GApplication (if we are one) to make sure at least trusted GNOME applications can be identified.
Created attachment 318492 [details] [review] forward: reverse: Add user-agent override propperty
Created attachment 318493 [details] [review] forward: reverse: Add user-agent override propperty
(In reply to Jonas Danielsson from comment #18) > Review of attachment 318483 [details] [review] [review]: > > Thanks a lot Philip! > > It feels a bit wrong to add pretty much the exactly same code and comments > to geocode-reverse.c as well (and geocode-reverse does not even have > properties at the moment, so it will add boilerplate code as well). > Do you or Bastian? see anyway around it? I know what you mean. I guess my two suggestions would be: • Factor it out into a GeocodeSession object which is accessed by both GeocodeForward and GeocodeReverse, much like SoupSession is for SoupRequests. But then you’d have a GeocodeSession object with only one property, and GeocodeForward:session and GeocodeReverse:session properties to boot. • Add it as a library-wide function, geocode_set_user_agent(), and not have it as a property at all. But that’s horribly non-object-oriented. Neither option feels great to me. I think I prefer the current state of the patches.
Review of attachment 318491 [details] [review]: Looks good to me.
Review of attachment 318493 [details] [review]: ::: geocode-glib/geocode-forward.c @@ +213,3 @@ + * If set to non-#NULL then @user_agent will override the default + * User Agent that is constructed from geocode-glib version and, + * if available, the applications #GApplication id. I would give a literal example of what the default is, i.e. ‘geocode-glib/3.20 (MyApplicationId)’. And this version still omits links to the Nominatim T&Cs and the HTTP spec for User Agents. @@ +1203,3 @@ + agent = _geocode_glib_user_agent (); + else + agent = (char *)user_agent; This will free the const input string, which is definitely wrong. I was thinking something more like: gchar *allocated_agent = NULL; if (user_agent == NULL) allocated_agent = user_agent = _geocode_glib_user_agent (); g_object_set (blah, user_agent, NULL); g_free (allocated_agent); @@ +1259,3 @@ + * + * Get the value of the #GeocodeForward:user-agent property. + * If the value is #NULL then the default User Agent is used. Missing a ‘Returns: ’ line. ::: geocode-glib/geocode-reverse.c @@ +52,3 @@ + PROP_0, + + PROP_USER_AGENT, To avoid creating an unused PROP_0 symbol, why not just do: enum { PROP_USER_AGENT = 1, }; @@ +131,3 @@ + * If set to non-#NULL then @user_agent will override the default + * User Agent that is constructed from geocode-glib version and, + * if available, the applications #GApplication id. Same problems as in GeocodeForward:user-agent. @@ +648,3 @@ + agent = _geocode_glib_user_agent (); + else + agent = (char *) user_agent; Same problems as in geocode_forward_set_user_agent(). @@ +661,3 @@ + * + * Get the value of the #GeocodeReverse:user-agent property. + * If the value is #NULL then the default User Agent is used. Same problems as in geocode_forward_get_user_agent().
Created attachment 318849 [details] [review] forward: reverse: Add user-agent override propperty
Created attachment 318850 [details] [review] forward: reverse: Add user-agent override propperty
Review of attachment 318850 [details] [review]: ::: geocode-glib/geocode-forward.c @@ +216,3 @@ + * + * The <ulink url="http://wiki.openstreetmap.org/wiki/Nominatim_usage_policy"> + * Nominatim usage policty</ulink> requires an User-Agent that s/policty/policy/ s/an User-Agent/a User-Agent/ @@ +218,3 @@ + * Nominatim usage policty</ulink> requires an User-Agent that + * identifies the application. + * For specification on the format of an User Agent, please see: <ulink s/an User Agent/a User Agent/ @@ +1265,3 @@ + * + * Get the value of the #GeocodeForward:user-agent property. + * If the value is #NULL then the default User Agent is used. s/#NULL/%NULL/ @@ +1267,3 @@ + * If the value is #NULL then the default User Agent is used. + * + * Returns: The current User Agent or %NULL if defeult is used. s/defeult/default/ Missing a ‘Since: 3.20’ line. @@ +1269,3 @@ + * Returns: The current User Agent or %NULL if defeult is used. + */ +char * This should be `const gchar *` or `const char *`. ::: geocode-glib/geocode-forward.h @@ +76,3 @@ void geocode_forward_set_bounded (GeocodeForward *forward, gboolean bounded); +char *geocode_forward_get_user_agent (GeocodeForward *forward); This should return `const gchar *` or `const char *`. ::: geocode-glib/geocode-reverse.c @@ +52,3 @@ + PROP_0, + + PROP_USER_AGENT, I would drop the PROP_0 here and use `PROP_USER_AGENT = 1,`, but they‘re equivalent. @@ +82,3 @@ + guint property_id, + const GValue *value, + GParamSpec *pspec) Whitespace and alignment is messed up in this parameter block. @@ +137,3 @@ + * identifies the application. + * For specification on the format of an User Agent, please see: <ulink + * url="http://www.ietf.org/rfc/rfc2616.txt">RFC2616</ulink> Same comments as for the documentation in GeocodeForward. @@ +669,3 @@ + * If the value is #NULL then the default User Agent is used. + * + * Returns: The current User Agent or %NULL if defeult is used. Same comments as for the documentation of geocode_forward_get_user_agent(). @@ +671,3 @@ + * Returns: The current User Agent or %NULL if defeult is used. + */ +char * Should be `const gchar *` or `const char *`. ::: geocode-glib/geocode-reverse.h @@ +71,3 @@ gpointer user_data); +char *geocode_reverse_get_user_agent (GeocodeReverse *reverse); Should return `const gchar *` or `const char *`.
Review of attachment 318491 [details] [review]: Pushed.
Created attachment 337657 [details] [review] geocode-nominatim: Add a user-agent property to override the User-Agent If an application wants to set a custom User-Agent, they can now do so via this property. This is probably sufficiently rarely needed that it can remain as a property of GeocodeNominatim, without a getter or setter, and without being exposed on GeocodeForward and GeocodeReverse.
Created attachment 337658 [details] [review] geocode-glib: Add another User-Agent fallback string If a GApplication is not available, see if the application name was set using g_set_application_name() — if so, use that in the User-Agent string to try and make it more unique.
Here are two patches which rework the patch from comment #26 to be on top of the patches from bug #756311 (which introduce GeocodeNominatim and GeocodeBackend).
Created attachment 343461 [details] [review] geocode-nominatim: Add a user-agent property to override the User-Agent If an application wants to set a custom User-Agent, they can now do so via this property. This is probably sufficiently rarely needed that it can remain as a property of GeocodeNominatim, without a getter or setter, and without being exposed on GeocodeForward and GeocodeReverse.
Created attachment 343462 [details] [review] geocode-glib: Add another User-Agent fallback string If a GApplication is not available, see if the application name was set using g_set_application_name() — if so, use that in the User-Agent string to try and make it more unique.
Review of attachment 343461 [details] [review]: ::: geocode-glib/geocode-glib.c @@ +47,3 @@ application = g_application_get_default (); + + if (user_agent_override != NULL) { If it's != NULL, there's no need to get the application above. @@ +48,3 @@ + + if (user_agent_override != NULL) { + user_agent = g_strdup (user_agent_override); And here there wouldn't be any need to duplicate the string, so move the soup_session_new_with_options() inside each branch of the conditional. ::: geocode-glib/geocode-nominatim.c @@ +1393,3 @@ break; + case PROP_USER_AGENT: + if (g_strcmp0 (priv->user_agent, g_value_get_string (value)) != 0) { I'd say this optimisation isn't needed. @@ +1396,3 @@ + g_free (priv->user_agent); + priv->user_agent = g_value_dup_string (value); + g_object_notify (object, "user-agent"); g_object_notify_by_pspec()? You have the pspec in properties[PROP_USER_AGENT]
Review of attachment 343462 [details] [review]: Nice.
Created attachment 343597 [details] [review] geocode-nominatim: Add a user-agent property to override the User-Agent If an application wants to set a custom User-Agent, they can now do so via this property. This is probably sufficiently rarely needed that it can remain as a property of GeocodeNominatim, without a getter or setter, and without being exposed on GeocodeForward and GeocodeReverse.
Created attachment 343598 [details] [review] geocode-glib: Add another User-Agent fallback string If a GApplication is not available, see if the application name was set using g_set_application_name() — if so, use that in the User-Agent string to try and make it more unique.
Review of attachment 343461 [details] [review]: ::: geocode-glib/geocode-nominatim.c @@ +1393,3 @@ break; + case PROP_USER_AGENT: + if (g_strcmp0 (priv->user_agent, g_value_get_string (value)) != 0) { It’s not an optimisation for allocations: it’s to prevent emitting notify::user-agent if the value doesn’t change. That’s standard GObject practice.
Review of attachment 343597 [details] [review]: ::: geocode-glib/geocode-nominatim.c @@ +1500,3 @@ + properties[PROP_USER_AGENT] = g_param_spec_string ("user-agent", + "User agent", + "User-Agent string to send with HTTP requests", HTTP(S) requests.
Review of attachment 343598 [details] [review]: Yep.
Ta. Pushed with that documentation wording fix. Attachment 343597 [details] pushed as d229c59 - geocode-nominatim: Add a user-agent property to override the User-Agent Attachment 343598 [details] pushed as befbd2f - geocode-glib: Add another User-Agent fallback string