atticuscornett / AtmosWeather

Atmos Weather is a lightweight weather app for receiving alerts and forecasts in the US.
https://atticuscornett.github.io/AtmosWeather/
GNU General Public License v3.0
24 stars 4 forks source link

Nominatim a.o. #5

Closed IzzySoft closed 1 year ago

IzzySoft commented 1 year ago

I just took a look at your nice app, but had some issues with it:

Neverthess, I've just added it to my repo, so it should show up here around 7 pm UTC with the next sync. Would be nice if some of the above issues could be tackled, so description can be "cleaned" of my remarks and maybe even the AntiFeatures be removed :wink:

Thanks in advance!

atticuscornett commented 1 year ago

Thanks for your interest in Atmos Weather! I hope the information below is helpful.

If you have any other questions, don't hesitate to ask!

IzzySoft commented 1 year ago

does not return data for locations outside the US

Ah, I almost guessed as much but wasn't sure. That's why the "location" also stayed empty here in Germany I guess. So my careful hint of "focused on the USA" was not too far off (your app is online in my repo now – and in my list of Weather Apps)…

I plan to clarify this in the README and description soon.

Much appreciated! If you wish I can send you the metadata I've set up here per PR in Fastlane structure, so you would be in control of how your app is presented (Fastlane is a "quasi standard" supported by several places – and with its binaries, if you wish to use those, can even be used to deploy to the official stores of Google and Apple). For guidance, you'd then be welcome to use my Fastlane Cheat Sheet.

I hope to expand the app outside the US someday

Looking forward to that, when the time comes! With the metadata under your control, description can reflect such changes without my intervention (btw: updates are pulled automatically by my updater, usually once a day, whenever there's a new release with an APK attached).

does not send any location information

Don't forget the IP address needed to get the connection established. Sufficient for a "coarse location". I'm not complaining, just pointing out – might be worth to mention that somewhere as well (maybe a separate "privacy section" on the Readme with the details, and a pointer to "look there" with the description/onboarding).

I will reevaluate everything written in the intro before the next update to improve clarity and make sure data isn't getting sent anywhere else.

:heart_eyes: Wonderful, thanks! And truly appreciated!!

This is definitely a bug and any additional information would be appreciated.

Wileyfox Swift running Android 10 (LineageOS) with microG (entirely Google-free; though microG supports device registration and FCM, that's disabled by default and I left it at that). For closer device specs… ugh, I haven't uploaded that to my list of examples it seems, as there are already enough Android-10 devices… OK, maybe this one helps? Let me know what other details you might need (and whether you prefer that part rather moved to a separate issue).

I plan to rework the location system in the future, but this probably will not happen before the next update.

It's ready when it's ready – I'm already very impressed by your willingness to tackle things and achieve max transparency :bow:

So thanks a lot!

atticuscornett commented 1 year ago

Apologies for the late response.

I have clarified in both the README and the repo description that Atmos Weather only works for US locations. (0c266d1)

I have also updated the welcome screen and privacy statement to clarify what services Atmos Weather connects to. (0fe9fc7) You can preview the statement on the browser version of Atmos Weather. It is located in settings. (Users are now directed to read this statement for more detailed information on the welcome screen.) However, you mentioned that you observed connections to 146.75.121.91 and Akamai. I do not know what these connections are or why they would be occurring. How did you observe these connections? Any further details would be appreciated.

As for the bug not allowing the popup to be closed, I am still working on tracking that down. That should probably be moved to a different issue, thanks.

Finally, I believe I have removed all uses of Google's GMS Location Services and replaced them with Android's LocationManager. (060bc86) I am going to do more testing with it to ensure there is no major impact on battery life and all location features still work, but it, fortunately, turned out to be an easier fix than I had originally anticipated.

IzzySoft commented 1 year ago

How did you observe these connections?

By running PCAPDroid (a network monitor available at F-Droid) on the device, looking for all connections initiated by the app.

I believe I have removed all uses of Google's GMS Location Services

Great to read, congrats!

I am going to do more testing

Sane thing to do, definitely – thanks for the effort taken!

it, fortunately, turned out to be an easier fix than I had originally anticipated.

My approach exactly: start with a pessimistic view – and you're either proven correct or positively surprised :see_no_evil: Glad to read!

atticuscornett commented 1 year ago

I am still unsure where the additional connections you mentioned are coming from. When running PCAPDroid, I only see the regular connections that Atmos makes. The only additional connection I found with PCAPDroid is a connection to content-autofill.googleapis.com, which is not initialized by Atmos and is just the autofill provider set in Android settings. Maybe those connections have to do with whatever autofill provider you use. Alternatively, maybe these addresses have to do with Google's network location provider previously used in Atmos. In that case, these connections are no longer made since I only use Android's native GPS now. Sorry to keep bugging you about this, but I want to make sure there isn't anything missing from the privacy section before I release the next version. Thanks!

IzzySoft commented 1 year ago

When running PCAPDroid, I only see the regular connections that Atmos makes

I can see that I make another test run when time permits. Just to compare: did you run PCAPDroid in rooted mode, or non-rooted (i.e. VPN mode)?

The only additional connection I found with PCAPDroid is a connection to content-autofill.googleapis.com, which is not initialized by Atmos and is just the autofill provider set in Android settings.

And usually happens with LineageOS via their WebView, I have an issue open there, yes.

Maybe those connections have to do with whatever autofill provider you use.

None. So should not be that (apart from the connections LOS/Webview usually establishes – but as I frequently perform app testing with F-Droid reviews, I know those culprits and filter them out in my brains).

Alternatively, maybe these addresses have to do with Google's network location provider previously used in Atmos. In that case, these connections are no longer made since I only use Android's native GPS now.

That might be a point (if those libs were included with Atmos; my devices all run on Google-free ROMs).

Sorry to keep bugging you about this, but I want to make sure there isn't anything missing from the privacy section before I release the next version.

:+1: :star_struck: A strong reason to run another test here then. Do you have an APK I should use for that?

Thanks!

My pleasure! Especially for devs that, like you, really care for privacy!

atticuscornett commented 1 year ago

Thanks again! I ran PCAPDroid in non-rooted (VPN) mode. The latest Atmos Weather build APK (which does not use Google Services) is located at https://github.com/atticuscornett/AtmosWeather/raw/main/platforms/android/app/release/app-release.apk.

IzzySoft commented 1 year ago

Hm, that version (1.0.2) claims a newer one (1.1.0) is available. But OK, let's take a look.

That's before any weather data was retrieved (Atmos still looking for european weather). As there is no map shown, I wonder why it queries OSM tile servers – but I wonder even more what those other two sites might be for. So I add a place I know the app is working for, and see some "funny" connections to IPv6 addresses (next to the expected Nominatim):

But no request to the weather site (.gov) despite of weather data showing up, so this must be the .gov weather site then – but no, 2a02:26f0:3500:586::116 resolves to Akamai:

$ nslookup 2a02:26f0:3500:586::116
6.1.1.0.0.0.0.0.0.0.0.0.0.0.0.0.6.8.5.0.0.0.5.3.0.f.6.2.2.0.a.2.ip6.arpa    name = g2a02-26f0-3500-0586-0000-0000-0000-0116.deploy.static.akamaitechnologies.com.

Funny. Immediately before accessing that we see a DNS request for api.weather.gov. OK, so they obviously use Akamai for caching :man_shrugging: Yupp:

$ nslookup api.weather.gov
Server:     192.168.101.5
Address:    192.168.101.5#53

Non-authoritative answer:
api.weather.gov canonical name = sancert.weather.gov.edgekey.net.
sancert.weather.gov.edgekey.net canonical name = e278.dscg.akamaiedge.net.
Name:   e278.dscg.akamaiedge.net
Address: 23.35.236.15
Name:   e278.dscg.akamaiedge.net
Address: 2a02:26f0:3500:590::116
Name:   e278.dscg.akamaiedge.net
Address: 2a02:26f0:3500:586::116

I ran PCAPDroid in non-rooted (VPN) mode.

I'm running it in root mode, so maybe that makes a difference, don't know. But still wondering about those requests to OSM tile servers: am I missing something here? I do not see any map shown.

And btw, cool you even include air quality! Never noticed that in any other weather app.

atticuscornett commented 1 year ago

Thanks for looking again!

that version (1.0.2) claims a newer one (1.1.0) is available

1.0.2 is the latest fully released version - but I was updating some configurations last night and triggered that notification unintentionally. Should be fixed now.

the "x" (closing button) this time works; no idea what happened there last time, or maybe that's just the version where you've fixed that.

I have no idea why the "x" button didn't work originally because I don't think I changed anything related to that, but glad to hear it works now.

Nominatim should tell you that, so maybe a corresponding hint to the user would be useful

I will keep that in mind and possibly add a hint in the future.

There are a few to OSM, but also to a site called unpkg.com, and a github.io site

The updated privacy statement (located in settings, it will be pointed out in the update notes) explains the unpkg.com and atticuscornett.github.io connections. Unpkg.com hosts the Leaflet map library. (I may bundle Leaflet into Atmos in the future, removing the need for this connection.) The github.io site is used for update checks and hosting county geometry for displaying weather alerts.

As there is no map shown, I wonder why it queries OSM tile servers

A map is pre-loaded in the background that is used for displaying alerts so that it doesn't have to be regenerated every time an alert is opened.

OK, so they obviously use Akamai for caching

Mystery solved! Thanks for the help!

I think with this all of the issues you have mentioned have been resolved, but just in case I missed something, I won't close this issue yet. Every issue that was originally mentioned should be fixed when I release version 1.1.0 (which should be ready very soon hopefully.) Let me know if you have any more questions or problems, and if not, feel free to close this issue.

IzzySoft commented 1 year ago

Mystery solved! Thanks for the help!

Gladly – and good we found it all well founded. Just one thing:

The github.io site is used for update checks

If that does mean update-checks for the app itself (as I noticed in-app already), it might be useful to have a foss-flavor coming without that should you wish to list your app at F-Droid.org. There you'd be asked for that (or to at least have that disabled by default, with a proper note on enabling) as a security measure: users installing apps from F-Droid expect updates coming from there, too, including the additional checks performed there. So if an update comes by other means (which would require reproducible builds as otherwise they cannot work due to signature mismatch), that must be pretty clear. TL;DR: might be a good idea to have a "kill-switch" for that ready then :wink:

I won't close this issue yet. Every issue that was originally mentioned should be fixed when I release version 1.1.0 (which should be ready very soon hopefully.) Let me know if you have any more questions or problems, and if not, feel free to close this issue.

If you wish to keep it open until v1.1.0 is out, to keep track of things, I don't want to interfere :wink: Are any updates needed to the app description? Maybe I should open a PR with the metadata I use in my repo (Fastlane style) so you can control that directly? See my Fastlane Cheat Sheet for some details (and be welcome to use it), and just let me know. It's always preferable to have that maintained by the project itself (F-Droid will ask for it anyway should you apply there).

atticuscornett commented 1 year ago

For clarification - github.io is just used to notify users that an update is available for Android, it does not actually automatically install the updates, so that shouldn't be an issue. (Automatic updates are only provided for the Windows version, not the Android or other desktop versions.) Good to know for future reference though!

I don't think any updates to the description are necessary, but it would be nice to have the metadata if you don't mind making that PR. Thanks!

IzzySoft commented 1 year ago

For clarification - github.io is just used to notify users that an update is available for Android, it does not actually automatically install the updates, so that shouldn't be an issue.

Ah, OK – that wouldn't collide with the F-Droid rules then directly. It would still be confusing, as when the user looks in their F-Droid client, the announced update might not yet be there (a build cycle at F-Droid usually takes 2+ days – and even my updater only runs once a day).

I don't think any updates to the description are necessary,

Fine with me then.

but it would be nice to have the metadata if you don't mind making that PR.

Sure, will do!

IzzySoft commented 1 year ago

Oof, didn't expect the repo being that big. Do you think it's really a good idea to have all the *.exe files in the repo (electron/dist/*)? Wouldn't they better be pruned from there as they rather belong attached to releases/ (where they probably are as well)? They just bloat the (source) repo unnecessarily, eating space and bandwidth from all (potential) contributors. Seeing the latest.yml there you probably use that for some automation; maybe better move that to a separate repo then? Just thinking aloud, it's of course your decision :wink:

atticuscornett commented 1 year ago

Whoops, that's a local test build folder I used during early testing when this repo was private to transfer some files between computers! I need to add that whole folder to the .gitignore file. I rarely add test builds to that folder anymore, so it doesn't have any real purpose. I definitely have a few areas I need to prune to shrink the repo size.

IzzySoft commented 1 year ago

Whoops, that's a local test build folder I used during early testing …

Haha, look what the cat dragged in :see_no_evil: Yeah, something one notices on an initial clone, but no longer thereafter.

I need to add that whole folder to the .gitignore file.

:+1: Good idea.

I definitely have a few areas I need to prune to shrink the repo size.

Thanks for taking care! Unfortunately, IIRC, real pruning means rewriting history and force-pushing, so it would "invalidate" all forks/clones due to a mismatch in history. According to Github, there are no active forks currently, though – so it might be a good idea to go ahead before some show up :wink:

atticuscornett commented 1 year ago

Thank you for your patience and assistance as I worked on the problems you mentioned and for the Fastlane metadata! Now that Atmos Weather v1.1.0 has been released (https://github.com/atticuscornett/AtmosWeather/releases/tag/v1.1.0), the original issues have all been addressed. As for the minor issues you brought up during the discussion (repo size and eternal loading on the current location), I have added them to my to-do list on Notion. Thanks again!