Ponytech / appstoreconnectapi

Python wrapper around Apple App Store Api
https://ponytech.net/projects/app-store-connect
MIT License
161 stars 74 forks source link

on the topic of anonymous data collection #18

Open orenmazor opened 4 years ago

orenmazor commented 4 years ago

my test suite uses requests_mock to control outgoing requests and this one surprised me when I integrated this library.

>       raise exceptions.NoMockAddress(request)
E       requests_mock.exceptions.NoMockAddress: No mock address: POST https://stats.ponytech.net/new-event

looking in the code: https://github.com/Ponytech/appstoreconnectapi/blob/b73d4314e2a9f9098f3287f57fff687563e70b28/appstoreconnect/api.py#L238-L255

I see I can disable this but you should make this opt in, or at least extremely visible in your README.md. I'm using this library to pull financial reports and if I didn't opt into my usage being tracked, I'd be pretty upset down the line to discover this.

I get that its anonymous, but I also didn't opt in to it.

ppawlak commented 4 years ago

Hey @orenmazor thank you for raising this subject.

When I implemented this I was really anxious about how people would react about this "feature" as I know many are using this library to deal with sensitive data. I eventually decided not to make it opt-out by default as I was afraid to get no data at all.

I added a note about it in the changelog but I guess your right, this is not visible enough and I'll update the example in the README to show how to turn it off.

My goals for data collection were mainly for 3 reasons:

Feel free (you or anyone else reading this) to share your thoughts or best practices on this topic.

ppawlak commented 4 years ago

I had updated the README: https://github.com/Ponytech/appstoreconnectapi/commit/2c06c90af1ade33be5ad702aa88897b4e0289f41

I am leaving the issue open for now to get more feedback.

orenmazor commented 4 years ago

@ppawlak I get what you're saying, and this is after all your codebase. I appreciate that you added the notice. We're a little more privacy focused, so I actually forked your library and removed that stats collection.

quick comment on your changes: why do you need the issuer ID? if you truly have to collect it, please hash it with something other than sha1 as it is not cryptographically secure anymore, like sha-256.

ppawlak commented 4 years ago

quick comment on your changes: why do you need the issuer ID? if you truly have to collect it, please hash it with something other than sha1 as it is not cryptographically secure anymore, like sha-256.

This is to know how many different organizations uses the library. Thanks for tips, I'll switch to sha-256 in the next version.

jberkel commented 4 years ago

there is a worrying trend of more and more libraries collecting data and/or performing unrequested update checks.

to get an approximation of usage data, wouldn't it be enough to simply analyze PyPI package download stats?

ppawlak commented 4 years ago

to get an approximation of usage data, wouldn't it be enough to simply analyze PyPI package download stats?

This is indeed what I first looked at..
But I don't think it is very relevant: what if a single organization CI makes a dozen downloads per day? It also doesn't gives me what python versions are used or what are the most used functions.

ironslob commented 3 years ago

This is an extremely worrying addition to any project, and certainly doesn't feel good. It certainly wasn't obvious to me that this would be included.

Would you consider making this opt-in? From a community and privacy perspective the "opt-in by default" approach leaves a sour taste in my mouth. Even the GDPR highlighted that this is a poor approach.

I can understand the desire for some analytics, but in this instance ask permission, not forgiveness.

ppawlak commented 3 years ago

@ironslob Thanks for your feedback and I totally understand your point.

Making this opt-out by default had been mentioned already but my feeling is, this is far different from a desktop app were you have a popup asking for permission at first launch. I may be wrong but I think we'd would just get no analytics at all here.

Anyway, I am just considering to remove this "feature" completely now, there are too many concerns. I think for version 1.0 which I hope will happen sooner than later.

MrChadMWood commented 2 years ago

@ppawlak

I understand this concern, and your post about no nagware like a desktop app would have is valid. Have you considered making dont_collect_stats a required parameter? You could even hardcode a small print() statement on every run if disabled--advising that they are not supporting the project by this manner of use.

ppawlak commented 1 year ago

@MrChadMWood thanks for the suggestion, a required parameter makes sense. I'll see if I make this change or completely remove this "feature" as mentioned earlier. I had not been active on the development recently but I hope to get back on it soon.