Skip to content
Snippets Groups Projects
New issue look: Off

Keep permission prefixes

    • Closed Issue created by username-removed-16558 @dschuermann

    F-Droid server strips "android.permission." from permissions in https://gitlab.com/fdroid/fdroidserver/blob/master/fdroidserver/update.py#L535 I suspect this was implemented to display them easily. However in the client we must again add this prefix, and only for permissions that had this previously, which is not a good way to handle permissions in my opinion, there could be corner cases...

    For now I implemented a hack in https://gitlab.com/fdroid/fdroidclient/commit/3eb1e69bd38c5a495fcdaa85117bda3477333c5b (part of https://gitlab.com/fdroid/fdroidclient/merge_requests/322) that adds this prefix if no dot is inside the String, which is really hacky.

    An example how it currently looks inside index.xml

    <permissions>BIND_APPWIDGET,READ_EXTERNAL_STORAGE,VIBRATE,READ_CONTACTS,RESTART_PACKAGES,WRITE_SETTINGS,CALL_PHONE,EXPAND_STATUS_BAR,GET_TASKS,READ_CALL_LOG,SET_WALLPAPER,WRITE_EXTERNAL_STORAGE,org.adw.launcher.permission.READ_SETTINGS,org.adw.launcher.permission.WRITE_SETTINGS,PERSISTENT_ACTIVITY,SET_WALLPAPER_HINTS</permissions>

    Can we change the behavior of F-Droid server to not remove this prefixes? Maybe adding a new tag and leave the old inside index.xml for backward compatibility for 6 month or something? Feedback/opinions how to fix this more properly?

    Designs

    Child items 0

    • No child items are currently open.

    Activity

    • username-removed-22388
      username-removed-22388 @mvdan ·
      Owner

      I agree that this was hacky on the server side and should probably not have happened. Then again, do we gain anything by implementing what you're proposing here? Your current method is hacky, I agree, but I can't think of any situation where it would cause problems.

      Changing it on the server side is definitely "the right thing to do", but I don't think it's worth the trouble and the backward compatibility issues if we're not going to gain much more than "I don't like this hacky code".

    • username-removed-16558
      username-removed-16558 @dschuermann ·
      Author Developer

      If an app defines a custom permission without any dot inside its name my hack fails. I agree that changing the server causes some trouble but I am very cautious in this case as this is related to permissions, which are security related.

      Edited by username-removed-16558
    • username-removed-22388
      username-removed-22388 @mvdan ·
      Owner

      If an app defines a custom permission without any dot inside its name my hack fails.

      Ah, that is a fair point.

      I agree that changing the server causes some trouble but I am very cautious in this case as this is related to permissions, which are security related.

      I agree then. Ping @CiaranG - I'm curious if saving space was the only reason behind stripping the prefix of Android permissions.

    • username-removed-20330
      username-removed-20330 @CiaranG ·
      Owner

      As far as I remember it was only for saving space.

    • username-removed-22388
      username-removed-22388 @mvdan ·
      Owner

      Sounds like we should do this then.

    • username-removed-22388 Changed title: Not remove permission prefixes → Keep permission prefixes

      Changed title: Not remove permission prefixes → Keep permission prefixes

    • username-removed-25042
      username-removed-25042 @pserwylo ·
      Maintainer

      This sounds reasonable. Space should not be a problem, because such long strings should compress very well. This is good news, given that it is unlikely we will come up with a fully backwards compatible solution without specifying a new metadata tag. Currently we have a <permission> tag (or something like that), and this will need to be duplicated to also have a <fullPermission> tag (or something similar) which includes the fully prefixed permission names.

      The two situations which need to be considered are as follows:

      • Old F-Droid clients reading from a repo which includes the full permissions.
      • New F-Droid clients reading from an old repo which doesn't support full permissions.

      The old clients with new metadata will attempt to prefix the "android.permission" string before the permission name. This is the reason that the old <permission> tag will need to remain.

      The new clients reading old metadata will first look for the <fullPermission> tag, and if it can't find it, fall back to the old behaviour of looking at the <permission> values and then prefixing them.

      If anybody has ideas for how to do this without implementing a sort-of-duplicate-looking tag such as <fullPermission>, that'd be great too - as long as it satisfies the two cases outlined above.

    • username-removed-22388
      username-removed-22388 @mvdan ·
      Owner

      The new clients reading old metadata will first look for the tag, and if it can't find it, fall back to the old behaviour of looking at the values and then prefixing them.

      We can also use the index version. Whatever is easier.

      If anybody has ideas for how to do this without implementing a sort-of-duplicate-looking tag such as , that'd be great too - as long as it satisfies the two cases outlined above.

      I don't think there's a way around the temporary duplication.

      Given that the permissions list isn't a security-sensitive part of the client - the actual installer still shows the permissions from the apk - and that it won't keep old clients from still functioning, I say we keep it around for half a year or 3 major client releases, whichever happens first. We did something similar for the old description tags.

    • username-removed-24982
      username-removed-24982 @eighthave ·
      Owner

      At this point, I think we should just parrot the XML used in AndroidManifest.xml instead of trying to figure out how to convert this. For example, index.xml would contain:

      
<package>
  <version>1.0.3</version>
  <versioncode>8</versioncode>
  <apkname>at.bitfire.cadroid_8.apk</apkname>
  <srcname>at.bitfire.cadroid_8_src.tar.gz</srcname>
  <hash type="sha256">0b32c8e4c24f08babe96e749bfec91013f60e0a4406c933f39a6c979442e8b9a</hash>
  <sig>e48f055476a1d8d608c2fc91dbdce51c</sig>
  <size>102335</size>
  <sdkver>14</sdkver><targetSdkVersion>19</targetSdkVersion>
  <added>2015-05-10</added
  <permissions>ACCESS_NETWORK_STATE,ACCESS_WIFI_STATE,INTERNET,GET_ACCOUNTS,MANAGE_ACCOUNTS,READ_SYNC_SETTINGS,WRITE_SYNC_SETTINGS,READ_EXTERNAL_STORAGE,INTERNET,WRITE_EXTERNAL_STORAGE</permissions>
    <uses-permission name="android.permission.ACCESS_NETWORK_STATE"/>
    <uses-permission name="android.permission.ACCESS_WIFI_STATE"/>
    <uses-permission name="android.permission.INTERNET"/>
    <uses-permission name="android.permission.GET_ACCOUNTS"/>
    <uses-permission name="android.permission.MANAGE_ACCOUNTS"/>
    <uses-permission name="android.permission.READ_SYNC_SETTINGS"/>
    <uses-permission name="android.permission.WRITE_SYNC_SETTINGS"/>
    <uses-permission maxSdkVersion="22" name="android.permission.AUTHENTICATE_ACCOUNTS"/>
    <uses-permission maxSdkVersion="18" name="android.permission.READ_EXTERNAL_STORAGE"/>
    <uses-permission maxSdkVersion="18" name="android.permission.WRITE_EXTERNAL_STORAGE"/>
    <uses-permission name="android.permission.READ_CONTACTS"/>
    <uses-permission name="android.permission.WRITE_CONTACTS"/>
    <uses-permission name="android.permission.READ_CALENDAR"/>
    <uses-permission name="android.permission.WRITE_CALENDAR"/>
    <uses-permission name="org.dmfs.permission.READ_TASKS"/>
    <uses-permission name="org.dmfs.permission.WRITE_TASKS"/>
</package>

      There is also some related discussion in #704 (closed) and #703 (closed)

    • username-removed-24982 Added ~16415 label

      Added ~16415 label

    • username-removed-24982 Milestone changed to %0.101

      Milestone changed to %0.101

    • username-removed-22388
      username-removed-22388 @mvdan ·
      Owner

      Why have them all nest directly from <package>? Mixing our own tags with manifest tags seems very confusing. I would keep any verbatim tags within a separate tag, like <manifest>.

    • username-removed-24982
      username-removed-24982 @eighthave ·
      Owner

      Really, we should be only using tags as described in the AndroidManifest.xml whenever possible, I see no need for alternate names for the same thing. <package> in index.xml is already quite close to <application> in the AndroidManifest.xml. But totally changing that is a much bigger project. Also, the Android classes like PackageInfo and ApplicationInfo do not map directly to what's in the AndroidManifest.xml so I don't think adding a <manifest> section would add anything except to make the XML and related parsing code more complicated.

    • username-removed-22388
      username-removed-22388 @mvdan ·
      Owner

      I know nobody wants to take on a new index format, but the xml format is slowly becoming a zombie. We should start from scratch at some point.

    • username-removed-24982
      username-removed-24982 @eighthave ·
      Owner

      Yeah it would be nice to have a good, clean data format for the index, but I don't think its worth the effort at the moment. Once we have a stable, full feature set, and we're looking to implement libraries or clients in other languages, then I think it would be worth it.

    • username-removed-22388 Milestone changed to %0.102

      Milestone changed to %0.102

    • username-removed-24982
      username-removed-24982 @eighthave ·
      Owner

      I think !402 (merged) covers this pretty well, for now at least. I think we will have quite reliable conversion to the full permission names using Pattern.compile("[A-Z_]+") to match the old fdroid stripping permission data. Yes, technical a permission called MY_PERMISSION would be valid, but I've never seen someone name their own permissions like that.

    • username-removed-24982 Status changed to closed

      Status changed to closed

    Please register or sign in to reply