Closed Bug 903439 Opened 11 years ago Closed 7 years ago

Need API keys for Geolocation and Safebrowsing to work in SeaMonkey

Categories

(SeaMonkey :: Build Config, defect)

defect
Not set
normal

Tracking

(seamonkey2.48 wontfix, seamonkey2.49esr affected, seamonkey2.50 wontfix, seamonkey2.51 wontfix, seamonkey2.52 affected, seamonkey2.53 fixed, seamonkey2.57esr fixed)

RESOLVED FIXED
Tracking Status
seamonkey2.48 --- wontfix
seamonkey2.49esr --- affected
seamonkey2.50 --- wontfix
seamonkey2.51 --- wontfix
seamonkey2.52 --- affected
seamonkey2.53 --- fixed
seamonkey2.57esr --- fixed

People

(Reporter: philip.chee, Assigned: ewong)

References

Details

(Keywords: sec-other, Whiteboard: SM2.53.1)

User Story

A Google API key is needed for Safebrowsing to work in SeaMonkey.
A Mozilla API key is needed for Geolocation to work in SeaMonkey.

Attachments

(7 files, 8 obsolete files)

1.14 KB, patch
neil
: review+
Details | Diff | Splinter Review
1.08 KB, patch
Callek
: review+
Details | Diff | Splinter Review
4.13 KB, patch
iannbugzilla
: review+
Details | Diff | Splinter Review
14.79 KB, patch
iannbugzilla
: review+
Details | Diff | Splinter Review
7.93 KB, patch
frg
: review+
Details | Diff | Splinter Review
1.14 KB, patch
Details | Diff | Splinter Review
3.89 KB, patch
frg
: review+
Details | Diff | Splinter Review
Bug 882485 added API keys support for the Google Location Service API. This means that Geolocation doesn't work for us unless and until we get our own API Key.

According to Dougt the plan is to eventually use the Operating System provided Geolocation service if available. But in the mean time we should see if getting a Google API key is possible or even practical.
See also Bug 883233 (Use API key for desktop builds) for build config changes.
Blocks: 994093
Blocks: 998787
I'm wondering - now that we have the pref in the UI at least for non-release builds, shouldn't geo.enabled be set to false until we have a solution here so that users don't get prompted for geolocation service which doesn't work? (right now, you get prompted but nothing happens.)
(In reply to rsx11m from comment #2)
> I'm wondering - now that we have the pref in the UI at least for non-release
> builds, shouldn't geo.enabled be set to false until we have a solution here
> so that users don't get prompted for geolocation service which doesn't work?
> (right now, you get prompted but nothing happens.)
Agreed!
Simple patch, should apply to comm-aurora and comm-beta as well (for 2.28).
Attachment #8457962 - Flags: review?(neil)
Comment on attachment 8457962 [details] [diff] [review]
Disable geolocation for now [comment #8]

(Might need comm-release approval to make 2.28)
Attachment #8457962 - Flags: review?(neil) → review+
Comment on attachment 8457962 [details] [diff] [review]
Disable geolocation for now [comment #8]

Review of attachment 8457962 [details] [diff] [review]:
-----------------------------------------------------------------

land this on 28 and below!
Attachment #8457962 - Flags: approval-comm-release+
Attachment #8457962 - Flags: approval-comm-beta+
Attachment #8457962 - Flags: approval-comm-aurora+
Thanks, comm-beta already merged into comm-release, thus please push for comm-release as well.
Whiteboard: [c-n: comm-central/aurora/beta/release][leave open]
Keywords: checkin-needed
Comment on attachment 8457962 [details] [diff] [review]
Disable geolocation for now [comment #8]

Obviously, this should be backed out again with the final patch once a solution is available.
Attachment #8457962 - Attachment description: Disable geolocation for now → Disable geolocation for now [comment #8]
Whiteboard: [leave open]
Blocks: 1172559
I have the key now.  Setting it up now.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Great news! I'll open a bug to activate the UI from bug 994093 for the release builds as well once we've verified that the setup is working (I can also remove the override there if you want).
(In reply to rsx11m from comment #9)
> Comment on attachment 8457962 [details] [diff] [review]
> Disable geolocation for now [comment #8]
> 
> Obviously, this should be backed out again with the final patch once a
> solution is available.

This override got removed by bug 1172559 attachment 8630068 [details] [diff] [review] already, even though it appears that the ultimate fix was adding the geo.wifi.uri pref. What makes me worry is that I never saw the doorhanger notifications for sites requesting the location, which was the motivation in comment #2 for disabling geo.enabled in the first place.

So, let's see what happens when the API key is added and if it works out of the box then.
Blocks: 1271091
Attachment #8749083 - Flags: review?(bugspam.Callek) → review+
Interesting - I found https://developer.mozilla.org/en-US/docs/Web/API/Geolocation/Using_geolocation and it works with both SeaMonkey 2.47a1 (my own Linux build) and even the 2.40 release (on Windows), which shows me almost the same location as Firefox 47.0 does. Thus, it seems to do something right.  :-)
(In reply to rsx11m from comment #13)
> What makes me worry is that I never saw the doorhanger notifications for sites requesting the
> location, which was the motivation in comment #2 for disabling geo.enabled in the first place.

That's quite likely because I have geo.enabled set false in my production profiles and apparently never ran into a geolocation site with a testing profile (thus using the default).
https://hg.mozilla.org/SeaMonkey/puppet/rev/f1a94d5fc947a21a87a77fec27c04a1e4e7f2cf1
Bug 903439 - Add Google api key for geolocation to Linux slaves. r=Callek
Comment on attachment 8749083 [details] [diff] [review]
[puppet] install google api key on linux slaves (v1)[checked-in]

Pushed to SeaMonkey/puppet:
https://hg.mozilla.org/SeaMonkey/puppet/rev/f1a94d5fc947
Attachment #8749083 - Attachment description: [puppet] install google api key on linux slaves (v1) → [puppet] install google api key on linux slaves (v1)[checked-in]
(In reply to Edmund Wong (:ewong) from comment #17)
> Comment on attachment 8749083 [details] [diff] [review]
> [puppet] install google api key on linux slaves (v1)[checked-in]
> 
> Pushed to SeaMonkey/puppet:
> https://hg.mozilla.org/SeaMonkey/puppet/rev/f1a94d5fc947

Merged to production.
ewong according to the last status meetings the google api key is on the server. I checked yesterdays 2.48a1 and the build does not currently use any api key. 

For Firefox this is done in the build configs eg. linux x86:

>> mozilla\browser\config\mozconfigs\linux32\common-opt

>> ac_add_options --with-google-api-keyfile=/builds/gapi.data
>> ac_add_options --with-mozilla-api-keyfile=/builds/mozilla-desktop-geoloc-api.key

I don't see any options in the Seamonkey build configs. Also geolocation seems to be using a mozilla key not a google key.

If you let me know the location of the key on the server I can provide a patch for the build configs.

Do we need to ask Mozilla about a geolocation key?
Flags: needinfo?(ewong)
(In reply to Frank-Rainer Grahl from comment #19)
> ewong according to the last status meetings the google api key is on the
> server. I checked yesterdays 2.48a1 and the build does not currently use any
> api key. 
> 
> For Firefox this is done in the build configs eg. linux x86:
> 
> >> mozilla\browser\config\mozconfigs\linux32\common-opt
> 
> >> ac_add_options --with-google-api-keyfile=/builds/gapi.data
> >> ac_add_options --with-mozilla-api-keyfile=/builds/mozilla-desktop-geoloc-api.key
> 
> I don't see any options in the Seamonkey build configs. Also geolocation
> seems to be using a mozilla key not a google key.

Ok.. colour me double confused.

I am guessing gapi.data != mozilla-desktop-geoloc-api.key.  If so,
mcsmurf only gave me the gapi.data one.

I'm really sorry everyone.  Totally dropped the ball on this one.

> 
> If you let me know the location of the key on the server I can provide a
> patch for the build configs.
> 
> Do we need to ask Mozilla about a geolocation key?

No.. I think we need to rope mcsmurf in.

:mcsmurf,  do we have a geolocation api key?
Flags: needinfo?(ewong) → needinfo?(bugzilla)
I will add the mozconfig changes.
Actually the google one is used for Safe Browsing so don't throw it away :) Should be tested prior to providing it in the nightlies.
Attached patch wip patch. (obsolete) — Splinter Review
Attached patch [mozconfigs] proposed patch (obsolete) — Splinter Review
these patches check if $topsrcdir/suite is a directory.  if so, 
source the suite/config/mozconfigs/mozconfig.{platform}.common to
get the gapi.data(which we have) and the geoloc-api.key (pending mcsmurf's response)
Attachment #8782496 - Attachment is obsolete: true
Attachment #8782504 - Flags: review?(bugspam.Callek)
(In reply to Edmund Wong (:ewong) from comment #24)
> Created attachment 8782504 [details] [diff] [review]
> [mozconfigs] proposed patch
> 
> these patches check if $topsrcdir/suite is a directory.  if so, 
> source the suite/config/mozconfigs/mozconfig.{platform}.common to
> get the gapi.data(which we have) and the geoloc-api.key (pending mcsmurf's
> response)

mcsmurf has replied and I have the key already.
Flags: needinfo?(bugzilla)
Attached patch [mozconfigs] use api.key (obsolete) — Splinter Review
This uses both the safebrowsing and geolocation key.
Attachment #8782504 - Attachment is obsolete: true
Attachment #8782504 - Flags: review?(bugspam.Callek)
Attachment #8785121 - Flags: review?(philip.chee)
this patch is only for the mock slaves (linux*).  copies the appropriate
keys to their respective directory.. /builds.

since Callek's on pto and doesn't accept reviews, it's now up to IanN or
RattyAway.. :)
Attachment #8785122 - Flags: review?(philip.chee)
Attachment #8785122 - Flags: review?(iann_bugzilla)
taking this to private bug as I might be have mentioned something private here.
Group: core-security-release
mcmurf mentioned that the gapi.data key is the same for both safebrowsing
and geolocation.  

so the configs patch basically copies the gapi.data to different
file names.  just so that if in the near future we have a separate
geolocation key,  I can just change it.
Comment on attachment 8785122 [details] [diff] [review]
[configs] copy api keys to their required dirs.

Seems to be correct r=me
Attachment #8785122 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8785121 [details] [diff] [review]
[mozconfigs] use api.key

>+++ b/suite/config/mozconfigs/mozconfig.macosx.common
>@@ -0,0 +1,9 @@
>+# Common statements that are applicable to both Linux32 and Linux64
Maybe too much copy and paste?

>+++ b/suite/config/mozconfigs/mozconfig.win.common
>@@ -0,0 +1,9 @@
>+# Common statements that are applicable to both Linux32 and Linux64
Maybe too much copy and paste?
Attachment #8785121 - Flags: review?(philip.chee) → feedback+
>>
mcmurf mentioned that the gapi.data key is the same for both safebrowsing
and geolocation.  
<<

Not correct at least not in Firefox. Completely different keys.
Attachment #8785122 - Attachment is obsolete: true
Attachment #8785122 - Flags: review?(philip.chee)
Attachment #8785194 - Flags: review?(philip.chee)
Attachment #8785194 - Flags: review?(iann_bugzilla)
(In reply to Edmund Wong (:ewong) from comment #33)
> Created attachment 8785194 [details] [diff] [review]
> [mozconfigs] copy api keys to the required dir.

Did you obsolete the correct patch?
Flags: needinfo?(ewong)
Attachment #8785122 - Attachment is obsolete: false
Flags: needinfo?(ewong)
Attachment #8785121 - Attachment is obsolete: true
(In reply to Ian Neal from comment #34)
> (In reply to Edmund Wong (:ewong) from comment #33)
> > Created attachment 8785194 [details] [diff] [review]
> > [mozconfigs] copy api keys to the required dir.
> 
> Did you obsolete the correct patch?

Yes.. :(
Comment on attachment 8785194 [details] [diff] [review]
[mozconfigs] copy api keys to the required dir.

As discussed on IRC, patch seems to be missing some mozconfigs and there is an inconsistency in the use of -d and -e in testing $topsrcdir/suite between various mozconfigs.
Attachment #8785194 - Flags: review?(iann_bugzilla) → review-
(In reply to Edmund Wong (:ewong) from comment #28)
> taking this to private bug as I might be have mentioned something private here.

What/where? I'd rather hide individual comments or attachments than this whole non-security bug.
Flags: needinfo?(ewong)
Keywords: sec-other
(In reply to Daniel Veditz [:dveditz] from comment #37)
> (In reply to Edmund Wong (:ewong) from comment #28)
> > taking this to private bug as I might be have mentioned something private here.
> 
> What/where? I'd rather hide individual comments or attachments than this
> whole non-security bug.

I figured I was mentioning about the api key and figured it would be
safer just in case it is mentioned somewhere.  Since I don't have
the ability to hide any single comment,  hiding these comments would
be the next best thing until I am certain nothing is disclosed.
Flags: needinfo?(ewong)
Comment on attachment 8785194 [details] [diff] [review]
[mozconfigs] copy api keys to the required dir.

>--- a/suite/config/mozconfigs/linux32/debug
>+++ b/suite/config/mozconfigs/linux32/debug
>@@ -1,9 +1,12 @@
> . "$topsrcdir/build/unix/mozconfig.linux32"
>+if [ -d "$topsrcdir/suite" ]; then
>+  . "$topsrcdir/suite/config/mozconfigs/mozconfig.linux.common
>+fi

This is missing a closing '"' at the end of the line.

>--- a/suite/config/mozconfigs/linux32/nightly
>+++ b/suite/config/mozconfigs/linux32/nightly
>@@ -1,9 +1,12 @@
> . "$topsrcdir/build/unix/mozconfig.linux32"
>+if [ -d "$topsrcdir/suite" ]; then
>+  . "$topsrcdir/suite/config/mozconfigs/mozconfig.linux.common
>+fi

>--- a/suite/config/mozconfigs/linux64/nightly
>+++ b/suite/config/mozconfigs/linux64/nightly
>@@ -1,9 +1,12 @@
> . "$topsrcdir/build/unix/mozconfig.linux"
>+if [ -d "$topsrcdir/suite" ]; then
>+  . "$topsrcdir/suite/config/mozconfigs/mozconfig.linux.common
>+fi

>--- a/suite/config/mozconfigs/linux64/release
>+++ b/suite/config/mozconfigs/linux64/release
>@@ -1,10 +1,13 @@
> no_tooltool=1
> . "$topsrcdir/build/unix/mozconfig.linux"
>+if [ -d "$topsrcdir/suite" ]; then
>+  . "$topsrcdir/suite/config/mozconfigs/mozconfig.linux.common
>+fi

dto.

>--- a/suite/config/mozconfigs/macosx-universal/nightly
>+++ b/suite/config/mozconfigs/macosx-universal/nightly
>@@ -1,15 +1,16 @@
> if test -e "$topsrcdir/suite/config/version.txt"; then
>   unset CC
>   unset CXX
>+  . $topsrcdir/suite/config/mozconfigs/mozconfig.macosx.common
> fi

no '"' here at all? Aren't spaces allowed in Mac OSX file names?
 

>--- a/suite/config/mozconfigs/win32/debug
>+++ b/suite/config/mozconfigs/win32/debug
>@@ -1,10 +1,13 @@
> . "$topsrcdir/build/mozconfig.win-common"
> . "$topsrcdir/build/mozconfig.common"
>+if [ -e "$topsrcdir/suite" ]; then
>+    . "$topsrcdir/suite/config/mozconfigs/mozconfig.win.common"
>+fi

This looks ok.
Attached patch [mozconfigs] proposed patch (obsolete) — Splinter Review
Attachment #8785194 - Attachment is obsolete: true
Attachment #8793156 - Attachment is obsolete: true
Attachment #8785194 - Flags: review?(philip.chee)
Attachment #8793157 - Flags: review?(iann_bugzilla)
Are these changes not needed for l10n-mozconfig or macosx64/* files?
Flags: needinfo?(ewong)
(In reply to Ian Neal from comment #42)
> Are these changes not needed for l10n-mozconfig or macosx64/* files?

Will get  a patch up soon.
Flags: needinfo?(ewong)
Comment on attachment 8793157 [details] [diff] [review]
[mozconfigs] copy api keys to the required dir.

Cancelling review request as waiting on new patch
Attachment #8793157 - Flags: review?(iann_bugzilla)
Attachment #8793157 - Attachment is obsolete: true
Attachment #8846537 - Flags: review?(iann_bugzilla)
User Story: (updated)
Summary: Need a Google API key for Geolocation to work in SeaMonkey → Need API keys for Geolocation and Safebrowsing to work in SeaMonkey
Can someone point out what format either key is in?
> Can someone point out what format either key is in?

Done via irc. google key is a string. geoloc key seems to be a uuid.
Comment on attachment 8846537 [details] [diff] [review]
[mozconfigs] proposed patch

LGTM

Is it needed for any other repo?
Attachment #8846537 - Flags: review?(iann_bugzilla) → review+
Depends on: 1352850
https://hg.mozilla.org/build/buildbot-configs/rev/1b0aba93e9f44eab30ad3c11a9b1c02b5eb7f82b
Bug 903439 - Config changes to move api keys to their respective places r=IanN
Attached patch 903439-apikey.patch (obsolete) — Splinter Review
I believe mozconfig.linux.common is currently unused. If not I need to change the patch. 

This goes on top of the config changes in bug 1381211
Attachment #8886869 - Flags: review?(iann_bugzilla)
Attachment #8886869 - Flags: review?(ewong)
Depends on: 1381211
Comment on attachment 8886869 [details] [diff] [review]
903439-apikey.patch

LGTM r=me
Attachment #8886869 - Flags: review?(iann_bugzilla) → review+
ewong could you also check:

https://hg.mozilla.org/build/buildbot-configs/rev/1b0aba93e9f44eab30ad3c11a9b1c02b5eb7f82b

There the key is named gapi.data. I did it per irc:

http://logs.glob.uno/?c=mozilla%23seamonkey&s=4+Jul+2017&e=4+Jul+2017

> 10:06	ewong|away	frg_away: for linux*/osx, use /builds/google-api.key 
> for windows do c:\builds\google-api.key
Attached patch 903439-apikey-V2.patch (obsolete) — Splinter Review
rebased patch for current checked in configs

r+ from IanN carried forward.
Attachment #8886869 - Attachment is obsolete: true
Attachment #8886869 - Flags: review?(ewong)
Attachment #8889512 - Flags: review+
Comment on attachment 8889512 [details] [diff] [review]
903439-apikey-V2.patch

ewong could you still look at the patch. Thanks
Flags: needinfo?(ewong)
Comment on attachment 8889512 [details] [diff] [review]
903439-apikey-V2.patch

Review of attachment 8889512 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.
Flags: needinfo?(ewong)
https://hg.mozilla.org/comm-central/rev/4cba71fe1365c01852aac2ac34d62064ecd44640

Lets see how this works out and then we can take it to the branches.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
proof that you can screw up one line without anyone noticing :)

/builds/slave/c-cen-t-osx64/build/suite/config/mozconfig.macosx.common: No such file or directory
/builds/slave/c-cen-t-lnx64/build/.mozconfig: line 2: /builds/slave/c-cen-t-lnx64/build/suite/config/mozconfig.linux.common: No such file or directory

Backout and repush
https://hg.mozilla.org/comm-central/rev/c9e047305620281f694b2ecd5a265f540ef0bdd3
https://hg.mozilla.org/comm-central/rev/6795cf66d9c88e8900e8b0ce1be5bf4090dd325c
Corrected patch for later branch checkin. r+ from IanN carried forward.
Attachment #8889512 - Attachment is obsolete: true
Attachment #8889991 - Flags: review+
The builds are now failing:

> checking for the Google API key... no
> ERROR: '/builds/google-api.key': No such file or directory.

ewong could you check the location and let me know if I should back out or fix it.
Status: RESOLVED → REOPENED
Flags: needinfo?(ewong)
Resolution: FIXED → ---
Disabled api key on Linux and OSX until this is solved.

https://hg.mozilla.org/comm-central/rev/97196ae9ba5a9e249cc6121ea67a0cfc9e580065

The api key is found on Windows. Looking at it it seems to be correct but does not work for V4. I am seeing a 400 error:

listmanager: 17:24:36 GMT+0200 (W. Europe Standard Time): download error for goog-phish-proto,goog-malware-proto,goog-unwanted-proto,goog-badbinurl-proto,goog-downloadwhite-proto: 400

Left it enabled for further evaluation of the problem.

FRG
Flags: needinfo?(ewong)
Attachment #8891898 - Flags: review?(frgrahl)
Comment on attachment 8891898 [details] [diff] [review]
[configs] fix for google-api.key.

r+ and thanks
Attachment #8891898 - Flags: review?(frgrahl) → review+
Comment on attachment 8891898 [details] [diff] [review]
[configs] fix for google-api.key.

Has this been pushed yet?
Flags: needinfo?(ewong)
reenabled the keys.
https://hg.mozilla.org/comm-central/rev/188dde86cd03c5b1c46b194f1f89df44e32e7ee7
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
If it sticks lets do new bug for problems with the key itself.
(In reply to Frank-Rainer Grahl (:frg) from comment #69)
> If it sticks lets do new bug for problems with the key itself.

Agreed.
Flags: needinfo?(ewong)
Group: core-security-release
Whiteboard: SM2.53.1
You need to log in before you can comment on or make changes to this bug.