Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: App is not downloading JSON data from configured Trusted Sources DHM_URGENT #430

Open
diarmidmackenzie opened this issue Apr 10, 2020 · 21 comments
Assignees
Labels
Bug Something isn't working High Priority High importance / High value Work In Progress Don't merge quite yet, still in progress

Comments

@diarmidmackenzie
Copy link

diarmidmackenzie commented Apr 10, 2020

I had been using the two sample HAs, with no indications of success from the app. I assumed that was because the GPS data didn't match my GPS location.

Later, I set up my own test HA hosted in Google Cloud, with geographically local data. On the "Choose health authority" page, I entered "Add authority by URL" then I entered the URL where my JSON data is hosted: https://safe-paths-273819.ew.r.appspot.com/infected.json

At this point, I expected the App to contact the server and pull the JSON data.

Since I have access to the server, I can check access logs, and I don't see the app even requesting the JSON data from my server, let alone validating it.

Discussed on Slack with Abhishek here, and he said he thought this was due to a bug that has already been identified in this area. Someone (I don't know who) is working on a fix.
https://covidsafepaths.slack.com/archives/C010RMS2EE9/p1586552412184200

Ken asked me to raise an issue to ensure this was tracked.

I'd love to add some diags or similar, but not sure what would be helpful.

@kenpugsley
Copy link
Collaborator

Thanks for filing this. Should be fixed in the next beta drop which will come out either tonight or tomorrow I think. Could use your help in verifying.

Just so we have the reference, @tremblerz is fixing a bunch of this code in #378

@diarmidmackenzie
Copy link
Author

Happy to test when you have a fix. Looking forward to finally seeing a big red X on my phone!

@diarmidmackenzie
Copy link
Author

@kenpugsley v0.9.2 still not available as far as I can see. Any idea when we'll jave something to test here?

@diarmidmackenzie
Copy link
Author

I believe we are on track here, but I think it's pretty important we fix before we push to the App Store.

@diarmidmackenzie diarmidmackenzie changed the title BUG: App is not downloading JSON data from configured Trusted Sources BUG: App is not downloading JSON data from configured Trusted Sources DHM_URGENT Apr 12, 2020
@diarmidmackenzie
Copy link
Author

@kenpugsley With v0.9.2 I am not seeing any different behaviour here. Even with local data from a URL like this, I am still not getting a match

http://diarmidmackenzie.pythonanywhere.com/infection-data?longitude=42&latitude=13

Were you expecting this to be fixed in v0.92, or are there still known issues?

@kenpugsley
Copy link
Collaborator

We'll have to replicate this issue to diagnose. I did expect that the issue would be resolved in 0.9.2, so we'll have to see what is going on. Adding the High Priority label.

@diarmidmackenzie
Copy link
Author

OK - I have checked, and I'm still not seeing any request hitting my web server access logs. So we are still not even getting as far as an outbound request.

What diagnostics can I provide from the app that would be useful? Can you build a debug build for me to run?

@penrods
Copy link
Contributor

penrods commented Apr 13, 2020

I addressed several things in #499 . Please recheck either that PR branch or after it is merged.

@diarmidmackenzie
Copy link
Author

diarmidmackenzie commented Apr 17, 2020

At v0.9.4 this is still not working. Still not seeing any HTTP requests in the server access logs. Therefore data is not even being requested (still).

Specifically:

My assumption is that there shoudl be an initial JSON data pull immediately that the HA is configured. Is that correct? I suppose it is possible that the data pull is delayed until the appropriate 12h download time... (if so, that's horrible from a testability point of view).

@kenpugsley
Copy link
Collaborator

I do not see this in my local testing. I see the requests hitting my servers. When I come back up for air from #529 I can try and take a look. @tremblerz are you able to look at this sooner than me?

@tremblerz
Copy link
Contributor

I am gonna add eula thing for now and will come back on this afterwards

@diarmidmackenzie
Copy link
Author

Is anyone lookign at this? I'm concerned that ken says this was working for him - I'm worried it might have been something about my setup causing problems. Available to collaborate on this in ay way that would help...

@diarmidmackenzie
Copy link
Author

I'm interested to know what the UX is supposed to be if you mistype a URL, or if you have no intenet connetcivity (e.g. airplane mode on) when you set up the HA.

It would be greeat to get feedback whether the HA had been contactable (especially for typos).

As it is, you just enter any string, and it goes on the list and you never get any feedback whether it was mistyped, whether data is downloading etc.

A better UX on various classes of failure might help us to diagnose this more quickly.

@kenpugsley
Copy link
Collaborator

I'd like to treat the UX issues here as a separate issue. I'll try to dig in tonight and see if I can replicate what's going on. I should be able to tell if I'm getting data from your server or not. Will update here.

I'm interested to know what the UX is supposed to be if you mistype a URL, or if you have no intenet connetcivity (e.g. airplane mode on) when you set up the HA.

It would be greeat to get feedback whether the HA had been contactable (especially for typos).

As it is, you just enter any string, and it goes on the list and you never get any feedback whether it was mistyped, whether data is downloading etc.

A better UX on various classes of failure might help us to diagnose this more quickly.

@kenpugsley
Copy link
Collaborator

OK... so plenty of egg on face here. As far as I can tell, the app should be downloading data correctly (though I can't explain why you don't see the hits in your logs). When I run the app in both the Android and iOS simulators I can see that the data is indeed being downloaded.

However, there are a couple of problems to deal with:

Problem 1: Your web service (http://diarmidmackenzie.pythonanywhere.com/infection-data?longitude=42&latitude=13) follows the Safe Places server documentation (https://github.com/tripleblindmarket/safe-places/blob/develop/Safe-Places-Server.md), but it turns out that documentation is incorrect. I just verified with @penrods, that a change was overlooked when the data structure was simplified some time ago. That doc will be fixed shortly. The correct JSON structure that Safe Places produces is shown in the example here: https://github.com/tripleblindmarket/safe-places/blob/develop/examples/safe-paths.json. Please try adjusting your service to follow that structure.

Problem 2: Because the app silently fails on any issue when loading health care authority data, the problem was about 100x harder to find. That's something we definitely need to prioritize for the v1 release. I'll talk with the dev team to prioritize adding an error message when the loading of health authority data fails to help diagnose these kinds of problems.

Thanks for continuing to push on this, and sorry it's taken so long to look at it in enough detail to figure out the problem. Please update on how it goes after adjusting your endpoint.

I'd like to keep this issue open to track the need for better UX on data load failures.

@diarmidmackenzie
Copy link
Author

OK - server updated to new format. I hope this is now correct?

http://diarmidmackenzie.pythonanywhere.com/infection-data?longitude=1&latitude=2

However I still don't seem to be able to reach it (and still not seeing any access logs). Does this now work with your app & my server?

Some questions & requests:

  1. Should I include http:// when I enter the URL or not? If it matters, please can we validate & reject the other?
  2. It's very difficult to tell if data is loaded or not. Not only is there no error on failure, but even on success the only way to verify is to have some location history in the same GPS locations & check for matches.... As well as an error on data load, would it be possible to add some diags somehere (maybe on the About page?) indicating
    (a) how many points of concern data loaded, and
    (b) how many points of my own location history are stored.

Both these would hugely help observability (and hence overall testability).

@kenpugsley
Copy link
Collaborator

OK - server updated to new format. I hope this is now correct?

Yes, it appears to work for me now. Structure looks correct, and I'm able to use it to cause a the "you may be exposed" to happen for me with my location.

However I still don't seem to be able to reach it (and still not seeing any access logs). Does this now work with your app & my server?

This I 'm still puzzled by. If you've not tried this already, can you do a share of your export history and open the file. Take the most recent lat/lon, and use that in the url for your test health authority. Make sure you match to least 4 decimal points. I think that should cause you to get an "exposed" state to trigger.

  1. Should I include http:// when I enter the URL or not? If it matters, please can we validate & reject the other?

Yes, you should include the http://. Agree that we should validate for proper url syntax.

  1. It's very difficult to tell if data is loaded or not. Not only is there no error on failure, but even on success the only way to verify is to have some location history in the same GPS locations & check for matches.... As well as an error on data load, would it be possible to add some diags somehere (maybe on the About page?) indicating
    (a) how many points of concern data loaded, and
    (b) how many points of my own location history are stored.

Agree with all of that. Will be talking to the dev team on what we should do here. I know having basic info would help not only people testing the app, but technical end users as well. Ideally we should also have a way for the user to know more easily that a health authority hasn't been reached, but not sure if that would make v1.0 or not.

@diarmidmackenzie
Copy link
Author

OK - one problem I was having is that the prefix has to be https://

Not http://
Not HTTPS://
Not Https://

My phone kept auto-correcting https:// to one of the last 2.

I don't think that's been the only problem - but it's been the most recent one. Now all working!

@diarmidmackenzie
Copy link
Author

@kenpugsley Do you want to keep this open to track all the UX issues we spotted?

Or shall we close & spin up more to cover.

Key points:

  • Enforce correct URL form. Ideally accept various capitalizations of http / https, but at the very least don't permit ones that don't work.
  • Give clear error handling in the event a URL is unreachable.
  • DIagnostics indication number of own location data points & infected location data points. E.g. on About screen?

@Patrick-Erichsen
Copy link
Contributor

@diarmidmackenzie I think we can close this out. Thoughts?

@diarmidmackenzie
Copy link
Author

Hi - just saw this update.

Even if the "add authority by URL" function won't be used much in the field, it's useful in test and it very hard to use due to the flaws above.

All the following would be very valueable:

  • Enforce correct URL form. Ideally accept various capitalizations of http / https, but at the very least don't permit ones that don't work.

  • Give clear error handling in the event a URL is unreachable.

  • Diagnostics indication number of own location data points & infected location data points. E.g. on About screen?

Can we raise a Jira to cover these?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working High Priority High importance / High value Work In Progress Don't merge quite yet, still in progress
Projects
None yet
Development

No branches or pull requests

6 participants