Change Autocrypt logic to more closely match the spec

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/SaveKeyringResult.java

@@ -24,12 +24,12 @@ import org.sufficientlysecure.keychain.pgp.CanonicalizedKeyRing;
 
 public class SaveKeyringResult extends OperationResult {
 
-    public final long mRingMasterKeyId;
+    public final Long savedMasterKeyId;
 
     public SaveKeyringResult(int result, OperationLog log,
                              CanonicalizedKeyRing ring) {
         super(result, log);
-        mRingMasterKeyId = ring != null ? ring.getMasterKeyId() : Constants.key.none;
+        savedMasterKeyId = ring != null ? ring.getMasterKeyId() : null;
     }
 
     // Some old key was updated
@@ -46,13 +46,13 @@ public class SaveKeyringResult extends OperationResult {
 
     public SaveKeyringResult(Parcel source) {
         super(source);
-        mRingMasterKeyId = source.readLong();
+        savedMasterKeyId = source.readLong();
     }
 
     @Override
     public void writeToParcel(Parcel dest, int flags) {
         super.writeToParcel(dest, flags);
-        dest.writeLong(mRingMasterKeyId);
+        dest.writeLong(savedMasterKeyId);
     }
 
     public static Creator<SaveKeyringResult> CREATOR = new Creator<SaveKeyringResult>() {

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/AutocryptPeerDataAccessObject.java

@@ -153,6 +153,23 @@ public class AutocryptPeerDataAccessObject {
         return null;
     }
 
+    public Date getLastSeenGossip(String autocryptId) {
+        Cursor cursor = queryInterface.query(ApiAutocryptPeer.buildByPackageNameAndAutocryptId(packageName, autocryptId),
+                null, null, null, null);
+
+        try {
+            if (cursor != null && cursor.moveToFirst()) {
+                long lastUpdated = cursor.getColumnIndex(ApiAutocryptPeer.GOSSIP_LAST_SEEN_KEY);
+                return new Date(lastUpdated);
+            }
+        } finally {
+            if (cursor != null) {
+                cursor.close();
+            }
+        }
+        return null;
+    }
+
     public void updateLastSeen(String autocryptId, Date date) {
         ContentValues cv = new ContentValues();
         cv.put(ApiAutocryptPeer.LAST_SEEN, date.getTime());

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainProvider.java

@@ -1114,42 +1114,25 @@ public class KeychainProvider extends ContentProvider {
                     break;
                 }
                 case AUTOCRYPT_PEERS_BY_PACKAGE_NAME_AND_TRUST_ID: {
-                    ContentValues actualValues = new ContentValues();
                     String packageName = uri.getPathSegments().get(2);
-                    actualValues.put(ApiAutocryptPeer.PACKAGE_NAME, packageName);
+                    String identifier = uri.getLastPathSegment();
-                    actualValues.put(ApiAutocryptPeer.IDENTIFIER, uri.getLastPathSegment());
+                    values.put(ApiAutocryptPeer.PACKAGE_NAME, packageName);
+                    values.put(ApiAutocryptPeer.IDENTIFIER, identifier);
 
-                    Long newLastSeen = values.getAsLong(ApiAutocryptPeer.LAST_SEEN);
+                    int updated = db.update(Tables.API_AUTOCRYPT_PEERS, values,
-                    if (newLastSeen != null) {
+                            ApiAutocryptPeer.PACKAGE_NAME + "=? AND " + ApiAutocryptPeer.IDENTIFIER + "=?",
-                        actualValues.put(ApiAutocryptPeer.LAST_SEEN, newLastSeen);
+                            new String[] { packageName, identifier });
-                        db.replace(Tables.API_AUTOCRYPT_PEERS, null, actualValues);
+                    if (updated == 0) {
-                        break;
+                        db.insertOrThrow(Tables.API_AUTOCRYPT_PEERS, null, values);
                     }
 
                     Long masterKeyId = values.getAsLong(ApiAutocryptPeer.MASTER_KEY_ID);
                     if (masterKeyId != null) {
-                        Long lastSeenKey = values.getAsLong(ApiAutocryptPeer.LAST_SEEN_KEY);
-                        Long preferEncryptState = values.getAsLong(ApiAutocryptPeer.IS_MUTUAL);
-
-                        actualValues.put(ApiAutocryptPeer.MASTER_KEY_ID, masterKeyId);
-                        actualValues.put(ApiAutocryptPeer.LAST_SEEN_KEY, lastSeenKey);
-                        actualValues.put(ApiAutocryptPeer.IS_MUTUAL, preferEncryptState);
-
-                        db.replace(Tables.API_AUTOCRYPT_PEERS, null, actualValues);
                         contentResolver.notifyChange(KeyRings.buildGenericKeyRingUri(masterKeyId), null);
-                        break;
                     }
-
                     Long gossipMasterKeyId = values.getAsLong(ApiAutocryptPeer.GOSSIP_MASTER_KEY_ID);
-                    if (gossipMasterKeyId != null) {
+                    if (gossipMasterKeyId != null && !gossipMasterKeyId.equals(masterKeyId)) {
-                        Long gossipLastSeenKey = values.getAsLong(ApiAutocryptPeer.GOSSIP_LAST_SEEN_KEY);
-
-                        actualValues.put(ApiAutocryptPeer.GOSSIP_MASTER_KEY_ID, gossipMasterKeyId);
-                        actualValues.put(ApiAutocryptPeer.GOSSIP_LAST_SEEN_KEY, gossipLastSeenKey);
-
-                        db.replace(Tables.API_AUTOCRYPT_PEERS, null, actualValues);
                         contentResolver.notifyChange(KeyRings.buildGenericKeyRingUri(gossipMasterKeyId), null);
-                        break;
                     }
 
                     break;

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/AutocryptInteractor.java

@@ -5,15 +5,17 @@ import java.io.IOException;
 import java.util.Date;
 
 import android.content.Context;
+import android.support.annotation.Nullable;
 
 import org.openintents.openpgp.AutocryptPeerUpdate;
 import org.openintents.openpgp.AutocryptPeerUpdate.PreferEncrypt;
 import org.sufficientlysecure.keychain.Constants;
+import org.sufficientlysecure.keychain.operations.results.SaveKeyringResult;
 import org.sufficientlysecure.keychain.pgp.UncachedKeyRing;
 import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException;
 import org.sufficientlysecure.keychain.provider.AutocryptPeerDataAccessObject;
 import org.sufficientlysecure.keychain.provider.KeyWritableRepository;
-import org.sufficientlysecure.keychain.util.Log;
+import timber.log.Timber;
 
 
 class AutocryptInteractor {
@@ -33,74 +35,100 @@ class AutocryptInteractor {
         this.keyWritableRepository = keyWritableRepository;
     }
 
-    void updateAutocryptPeerState(String autocryptPeerId, AutocryptPeerUpdate autocryptPeerUpdate)
+    void updateAutocryptPeerState(String autocryptPeerId, AutocryptPeerUpdate autocryptPeerUpdate) {
-            throws PgpGeneralException, IOException {
-        if (autocryptPeerUpdate == null) {
-            return;
-        }
-
-        Long newMasterKeyId;
-        if (autocryptPeerUpdate.hasKeyData()) {
-            UncachedKeyRing uncachedKeyRing = UncachedKeyRing.decodeFromData(autocryptPeerUpdate.getKeyData());
-            if (uncachedKeyRing.isSecret()) {
-                Log.e(Constants.TAG, "Found secret key in autocrypt id! - Ignoring");
-                return;
-            }
-            // this will merge if the key already exists - no worries!
-            keyWritableRepository.savePublicKeyRing(uncachedKeyRing);
-            newMasterKeyId = uncachedKeyRing.getMasterKeyId();
-        } else {
-            newMasterKeyId = null;
-        }
-
         Date effectiveDate = autocryptPeerUpdate.getEffectiveDate();
-        autocryptPeerDao.updateLastSeen(autocryptPeerId, effectiveDate);
 
-        if (newMasterKeyId == null) {
+        // 1. If the message’s effective date is older than the peers[from-addr].autocrypt_timestamp value, then no changes are required, and the update process terminates.
+        Date lastSeenAutocrypt = autocryptPeerDao.getLastSeenKey(autocryptPeerId);
+        if (lastSeenAutocrypt != null && lastSeenAutocrypt.after(effectiveDate)) {
             return;
         }
 
-        Date lastSeenKey = autocryptPeerDao.getLastSeenKey(autocryptPeerId);
+        // 2. If the message’s effective date is more recent than peers[from-addr].last_seen then set peers[from-addr].last_seen to the message’s effective date.
-        if (lastSeenKey == null || effectiveDate.after(lastSeenKey)) {
+        Date lastSeen = autocryptPeerDao.getLastSeen(autocryptPeerId);
-            boolean isMutual = autocryptPeerUpdate.getPreferEncrypt() == PreferEncrypt.MUTUAL;
+        if (lastSeen == null || lastSeen.after(effectiveDate)) {
-            autocryptPeerDao.updateKey(autocryptPeerId, effectiveDate, newMasterKeyId, isMutual);
+            autocryptPeerDao.updateLastSeen(autocryptPeerId, effectiveDate);
         }
+
+        // 3. If the Autocrypt header is unavailable, no further changes are required and the update process terminates.
+        if (!autocryptPeerUpdate.hasKeyData()) {
+            return;
+        }
+
+        SaveKeyringResult saveKeyringResult = parseAndImportAutocryptKeyData(autocryptPeerUpdate);
+        if (saveKeyringResult == null) {
+            return;
+        }
+
+        // 4. Set peers[from-addr].autocrypt_timestamp to the message’s effective date.
+        // 5. Set peers[from-addr].public_key to the corresponding keydata value of the Autocrypt header.
+        Long newMasterKeyId = saveKeyringResult.savedMasterKeyId;
+        // 6. Set peers[from-addr].prefer_encrypt to the corresponding prefer-encrypt value of the Autocrypt header.
+        boolean isMutual = autocryptPeerUpdate.getPreferEncrypt() == PreferEncrypt.MUTUAL;
+
+        autocryptPeerDao.updateKey(autocryptPeerId, effectiveDate, newMasterKeyId, isMutual);
     }
 
-
+    void updateAutocryptPeerGossipState(String autocryptPeerId, AutocryptPeerUpdate autocryptPeerUpdate) {
-    void updateAutocryptPeerGossipState(String autocryptPeerId, AutocryptPeerUpdate autocryptPeerUpdate)
-            throws PgpGeneralException, IOException {
-        if (autocryptPeerUpdate == null) {
-            return;
-        }
-
-        Long newMasterKeyId;
-        if (autocryptPeerUpdate.hasKeyData()) {
-            UncachedKeyRing uncachedKeyRing = UncachedKeyRing.decodeFromData(autocryptPeerUpdate.getKeyData());
-            if (uncachedKeyRing.isSecret()) {
-                Log.e(Constants.TAG, "Found secret key in autocrypt id! - Ignoring");
-                return;
-            }
-            // this will merge if the key already exists - no worries!
-            keyWritableRepository.savePublicKeyRing(uncachedKeyRing);
-            newMasterKeyId = uncachedKeyRing.getMasterKeyId();
-        } else {
-            newMasterKeyId = null;
-        }
-
-        Date lastSeen = autocryptPeerDao.getLastSeen(autocryptPeerId);
         Date effectiveDate = autocryptPeerUpdate.getEffectiveDate();
-        if (newMasterKeyId == null) {
+
+        // 1. If gossip-addr does not match any recipient in the mail’s To or Cc header, the update process terminates (i.e., header is ignored).
+        // -> This should be taken care of in the mail client that sends us this data!
+
+        // 2. If peers[gossip-addr].gossip_timestamp is more recent than the message’s effective date, then the update process terminates.
+        Date lastSeenGossip = autocryptPeerDao.getLastSeenGossip(autocryptPeerId);
+        if (lastSeenGossip != null && lastSeenGossip.after(effectiveDate)) {
             return;
         }
 
-        Date lastSeenKey = autocryptPeerDao.getLastSeenKey(autocryptPeerId);
+        if (!autocryptPeerUpdate.hasKeyData()) {
-        if (lastSeenKey != null && effectiveDate.before(lastSeenKey)) {
             return;
         }
 
-        if (lastSeen == null || effectiveDate.after(lastSeen)) {
+        SaveKeyringResult saveKeyringResult = parseAndImportAutocryptKeyData(autocryptPeerUpdate);
-            autocryptPeerDao.updateKeyGossip(autocryptPeerId, effectiveDate, newMasterKeyId);
+        if (saveKeyringResult == null) {
+            return;
         }
+
+        // 3. Set peers[gossip-addr].gossip_timestamp to the message’s effective date.
+        // 4. Set peers[gossip-addr].gossip_key to the value of the keydata attribute.
+        Long newMasterKeyId = saveKeyringResult.savedMasterKeyId;
+
+        autocryptPeerDao.updateKeyGossip(autocryptPeerId, effectiveDate, newMasterKeyId);
+    }
+
+    @Nullable
+    private SaveKeyringResult parseAndImportAutocryptKeyData(AutocryptPeerUpdate autocryptPeerUpdate) {
+        UncachedKeyRing uncachedKeyRing = parseAutocryptKeyData(autocryptPeerUpdate);
+        if (uncachedKeyRing != null) {
+            return importAutocryptKeyData(uncachedKeyRing);
+        }
+        return null;
+    }
+
+    @Nullable
+    private SaveKeyringResult importAutocryptKeyData(UncachedKeyRing uncachedKeyRing) {
+        SaveKeyringResult saveKeyringResult = keyWritableRepository.savePublicKeyRing(uncachedKeyRing);
+        if (!saveKeyringResult.success()) {
+            Timber.e(Constants.TAG, "Error inserting key - ignoring!");
+            return null;
+        }
+        return saveKeyringResult;
+    }
+
+    @Nullable
+    private UncachedKeyRing parseAutocryptKeyData(AutocryptPeerUpdate autocryptPeerUpdate) {
+        UncachedKeyRing uncachedKeyRing;
+        try {
+            uncachedKeyRing = UncachedKeyRing.decodeFromData(autocryptPeerUpdate.getKeyData());
+        } catch (IOException | PgpGeneralException e) {
+            Timber.e(Constants.TAG, "Error parsing public key! - Ignoring");
+            return null;
+        }
+        if (uncachedKeyRing.isSecret()) {
+            Timber.e(Constants.TAG, "Found secret key in autocrypt id! - Ignoring");
+            return null;
+        }
+        return uncachedKeyRing;
     }
 }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java

@@ -762,7 +762,9 @@ public class OpenPgpService extends Service {
                 String autocryptPeerId = data.getStringExtra(OpenPgpApi.EXTRA_AUTOCRYPT_PEER_ID);
                 AutocryptPeerUpdate autocryptPeerUpdate = data.getParcelableExtra(OpenPgpApi.EXTRA_AUTOCRYPT_PEER_UPDATE);
 
-                autocryptInteractor.updateAutocryptPeerState(autocryptPeerId, autocryptPeerUpdate);
+                if (autocryptPeerUpdate != null) {
+                    autocryptInteractor.updateAutocryptPeerState(autocryptPeerId, autocryptPeerUpdate);
+                }
             }
 
             if (data.hasExtra(OpenPgpApi.EXTRA_AUTOCRYPT_PEER_GOSSIP_UPDATES)) {
@@ -770,7 +772,9 @@ public class OpenPgpService extends Service {
                 for (String address : updates.keySet()) {
                     Timber.d(Constants.TAG, "Updating gossip state: " + address);
                     AutocryptPeerUpdate update = updates.getParcelable(address);
-                    autocryptInteractor.updateAutocryptPeerGossipState(address, update);
+                    if (update != null) {
+                        autocryptInteractor.updateAutocryptPeerGossipState(address, update);
+                    }
                 }
             }