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

Introduce read-only ContentProvider for cards #1121

Merged
merged 2 commits into from Jul 3, 2023

Conversation

joserebelo
Copy link
Contributor

@joserebelo joserebelo commented Oct 30, 2022

Implement a simple ContentProvider that provides read-only access to the cards. This allows for external apps to read the cards and groups.

Read access is protected by the dangerous permission me.hackerchick.catima.READ_CARDS

Gadgetbridge PR with the matching integration (still very much a WIP for experimenting): https://codeberg.org/Freeyourgadget/Gadgetbridge/pulls/2953

Marking as draft since:

  • Only for feedback / visibility for now
  • Needs some docs
  • Need to check if and how to create some tests

@joserebelo
Copy link
Contributor Author

joserebelo commented Oct 31, 2022

It's obviously a problem that this would be exposing the internal DB structure, so breaking changes on it break the ContentProvider (#1046 comes immediately to mind).

I'll probably introduce some translation layer using the DBHelper to prevent that.

Alternatively, I can wait for #1046 to get merged and assume the DB structure will remain stable after that (new columns are non-breaking)

@TheLastProject
Copy link
Member

This is quite cool! Feels much cleaner than my intent idea, thanks for picking this up!

The insert, update and delete methods returning 0 seems quite weird to me. Is this part of the default ContentProvider contract, to return the amount of entries edited and therefore you're always just returning 0 because only reading is allowed? I think having a comment there explaining it would be help (how would making it writable work? How would it check the permission?)

Alternatively, I can wait for #1046 to get merged and assume the DB structure will remain stable after that (new columns are non-breaking)

I think waiting for #1046 first makes sense. Aside from that it should be fairly safe to assume things will be non-breaking as the database rarely changes.

However, just to be future proof, we may want to consider adding some kind of API level somewhere and put some thin layer between the database and the output so that if we ever have a breaking database change that could be breaking we could have a translation layer ensure the API won't change.

@joserebelo
Copy link
Contributor Author

Is this part of the default ContentProvider contract, to return the amount of entries edited and therefore you're always just returning 0 because only reading is allowed?

Yes, it's recommended in the documentation:

Although you must implement these methods, your code does not have to do anything except return the expected data type. For example, you may want to prevent other applications from inserting data into some tables. To do this, you can ignore the call to insert() and return 0.

I'll add a comment mentioning this.

how would making it writable work? How would it check the permission?

There's an extra android:writePermission. I haven't tried it, but I'm assuming it gets checked automatically by Android when accessing the write methods, just as the readPermission did for the query. Now that you mention it, since I haven't declared it, I'm not sure if any app would be able to call the write methods as-is, even though they don't do anything 🤔

Maybe it wouldn't be a bad idea to add the permission right now anyway.

However, just to be future proof, we may want to consider adding some kind of API level somewhere and put some thin layer between the database and the output so that if we ever have a breaking database change that could be breaking we could have a translation layer ensure the API won't change.

Yup, it's always better to have some sort of API versioning before you actually need it. I'll add a simple /version uri with a single record containing a major + minor.

@joserebelo
Copy link
Contributor Author

joserebelo commented Jun 18, 2023

@TheLastProject what are your thoughts regarding merging this before the group IDs change?

I can just add the version uri, assume that the breaking change will happen eventually, and add that to the content provider documentation.

I was looking into adding the layer to keep it backwards compatible, but it's not trivial to generate stable IDs.

@TheLastProject
Copy link
Member

I think that sounds fine, for now the only users will likely be Gadgetbridge and those users can probably deal with that happening in the future. Just needs some kind of version URI yeah, so integrations can deal with this.

@joserebelo joserebelo force-pushed the gadgetbridge branch 2 times, most recently from d9fada9 to 4e7db90 Compare June 18, 2023 20:00
@joserebelo joserebelo marked this pull request as ready for review June 18, 2023 20:06
@joserebelo
Copy link
Contributor Author

Updated:

  • Added docs
  • Added a thin layer to hide internal columns (zoom level does not seem relevant)
  • Added some basic unit tests to detect breaking changes

Should be ready for a first review.

@joserebelo joserebelo force-pushed the gadgetbridge branch 3 times, most recently from 1c3ad72 to ef7aa41 Compare June 18, 2023 20:48
@TheLastProject
Copy link
Member

Just for my understanding for when I finally find the time to review this (life has been absurdly busy the past month), is https://codeberg.org/Freeyourgadget/Gadgetbridge/pulls/2953 still WIP because the Catima side hasn't been merged yet or because you still expect more changes to be necessary?

@joserebelo
Copy link
Contributor Author

@TheLastProject no rush. The pull request on GB is marked as wip mostly because this hasn't been merged yet, just in case we end up doing any breaking changes here.

Copy link
Member

@TheLastProject TheLastProject left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the code seems clear enough and none of it touches the rest of Catima so I'm not really worried about my ability to maintain it (especially because Gadgetbridge will likely be the main user).

I do think it might be good to add an option in settings for "Allow other apps to access my data" so those paranoid can disable this feature completely.

How does requesting for this go? I suppose this is a runtime permission, not an install-time permission?

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
android:description="@string/permissionReadCardsDescription"
android:icon="@drawable/ic_launcher_foreground"
android:label="@string/permissionReadCardsLabel"
android:name="me.hackerchick.catima.READ_CARDS"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the docs you write:

The authority for this content provider: `<package_name>.contentprovider.cards`

There are 3 release channels, with 2 possible package names:

| Release Channel | Package Name                |
|-----------------|-----------------------------|
| Google Play     | me.hackerchick.catima       |
| F-Droid         | me.hackerchick.catima       |
| Debug Build     | me.hackerchick.catima.debug |

But it seems to me like this will not use me.hackerchick.catima.debug.READ_CARDS in the debug channel?

I don't really mind apps not having to request both, but IIRC this could cause installation conflicts from the regular and debug builds, not allowing both at the same time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but IIRC this could cause installation conflicts from the regular and debug builds, not allowing both at the same time?

That only happens if there's a conflict between content provider authorities. In the AndroidManifest I set the authority to ${applicationId}.contentprovider.cards, so both channels can be installed in parallel.

I did forget about the permission. Just tested it, and granting it once on either channel grants it on both apps. Do you think this is OK?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine to me, yeah

@joserebelo
Copy link
Contributor Author

Reading the code seems clear enough and none of it touches the rest of Catima so I'm not really worried about my ability to maintain it (especially because Gadgetbridge will likely be the main user).

I would also be up for fixing issues that come up - just reach out if I do miss any issue :)

I do think it might be good to add an option in settings for "Allow other apps to access my data" so those paranoid can disable this feature completely.

Should this be disabled by default? If enabled by default (or even if not), I'll probably also include a summary with Apps will still have to request permissions to access the data, otherwise it sounds like access is just free for all.

How does requesting for this go? I suppose this is a runtime permission, not an install-time permission?

That is correct - this is a runtime permission.

@TheLastProject
Copy link
Member

I would also be up for fixing issues that come up - just reach out if I do miss any issue :)

Sounds good :)

Should this be disabled by default? If enabled by default (or even if not), I'll probably also include a summary with Apps will still have to request permissions to access the data, otherwise it sounds like access is just free for all.

That is correct - this is a runtime permission.

Given it is a runtime permission and people have to explicitly allow it (and it's not just silently allowed when installing a new app) I think enabling it by default is fine. If there is any reason to believe people are being tricked into allowing it from malicious apps we can always start disallowing it by default. Adding your note sounds good.

@joserebelo
Copy link
Contributor Author

joserebelo commented Jul 1, 2023

@TheLastProject small problem - runtime permissions were only included in Android 6.0 (api level 23) and above. Catima's minSdkVersion is 21, just like Gadgetbridge, so the permission is granted silently on Android 5.0. I don't really see a way around this other than disabling the content provider on api level < 23.

@TheLastProject
Copy link
Member

Hmm, that's a tough one. Looking at Google Play there are currently 121 Catima users on 5.0 and 5.1, about 0.3% of users. Of course, I have no statistics for F-Droid, but even F-Droid itself dropped Android 5 support recently.

So, given the following:

  1. I believe data privacy is more important than new features
  2. The amount of Android 5 users is dropping constantly and at ~0.3% currently (Google Play statistics)
  3. We are going to have to drop Android 5 support at some point (we're already dealing with some Catima dependencies dropping support like Bump org.apache.commons:commons-csv from 1.9.0 to 1.10.0 #1204)

I think it makes sense to do the minimal amount of work to keep Android 5 user's data safe and enable the new feature for everyone else. So, just not having the feature on Android 5 sounds like the best way to go. Given Catima is open source, if some Android 5 user really really really wants this feature and understands the risks they could rebuild it with the feature enabled.

@joserebelo
Copy link
Contributor Author

Disabled the content provider and hid the preference on SDK < 23.

@TheLastProject TheLastProject merged commit bf94d20 into CatimaLoyalty:main Jul 3, 2023
2 checks passed
@TheLastProject
Copy link
Member

Thanks!

@joserebelo joserebelo deleted the gadgetbridge branch July 3, 2023 18:59
@obfusk
Copy link
Member

obfusk commented Jul 3, 2023

This looks nice! I do wonder what will happen when/if we finally implement proper group IDs?

I guess we could keep the current API and simply hide the IDs from the content provider. Is it possible to implement both that and a newer version of the API that does include it or can we only provide one API version at a time? Or is that just not worth it compared to the relatively trivial change to just use the updated API?

Just wondering if this is something to keep in mind for anyone using the content provider or something to worry about later :)

@joserebelo
Copy link
Contributor Author

@obfusk when that happens, I was thinking of just going with the breaking change and bumping the API to 2.0.

I guess we could keep the current API and simply hide the IDs from the content provider.

True, but the semantics might change slightly. Right now, the _id of groups is actually the group name, and later it will map to an actual ID.

While we could definitely maintain 2 versions of the content provider, I don't think it's worth the extra effort.

Future breaking changes can probably be prevented by manipulating the data on the content provider, but these group ID changes are trickier.

@TheLastProject
Copy link
Member

Given right now Gadgetbridge is the only consumer of this and Gadgetbridge users tend to be fairly tech-savvy I think breaking this is okay especially because we can coordinate releases somewhat. Long term we probably do want to look at backwards compatibility (at least temporarily). But waiting for perfection already delayed this feature for almost a year so it seemed better to get something useful out of the door :)

By the way, please ping me when screenshots or news or so about this feature gets published anywhere, I'd love to see how this works (not having Zepp OS myself, just Bangle.js) :)

@joserebelo
Copy link
Contributor Author

@TheLastProject some screenshots below. It's nothing fancy, just a simple preferences screen, but I think it serves as pretty usable MVP.

I was thinking of proposing some intent broadcast from Catima when cards are added / updated, so that Gadgetbridge can get notified of changes and sync the cards accordingly, but haven't thought too much into that yet.

Screenshots

Catima installed

catima_installed

Catima installed, but not compatible (no content provider / incompatible version)

catima_not_compatible

Catima not installed

catima_not_installed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants