1Password: correctly detect invalid vault and item ID

src/OnePassword/Client.cs

@@ -79,27 +79,50 @@ public static Vault OpenVault(VaultInfo info, Session session)
 
         public static OneOf<Account, SshKey, NoItem> GetItem(string itemId, string vaultId, Session session)
         {
-            var oneOf3 = GetVaultItem(itemId, vaultId, session.Keychain, session.Key, session.Rest);
+            // 1. On the first request we fetch everything we need to decrypt the item. This is also allows us to check
+            // upfront if the vault exists and if it's accessible.
+            if (session.AccountInfo == null)
+            {
+                var accountInfo = GetAccountInfo(session.Key, session.Rest);
+                var keysetsInfo = GetKeysets(session.Key, session.Rest);
 
-            // The item is not available
-            if (oneOf3.TryPickT2(out var failure, out var oneOf2))
-                return failure;
+                // Decrypt into the session keychain
+                DecryptKeysets(keysetsInfo.Keysets, session.Credentials, session.Keychain);
 
-            var item = oneOf2.Match<VaultItem>(a => a, k => k);
+                // Figure out which vaults are accessible with the current keychain
+                var accessibleVaults = GetAccessibleVaults(accountInfo, session.Keychain).ToArray();
 
-            if (CanDecrypt(item))
-                return oneOf3;
+                // Decrypt all the vault keys into the session keychain
+                foreach (var v in accessibleVaults)
+                    v.DecryptKeyIntoKeychain();
+
+                // Store last to ensure consistent state
+                session.AccountInfo = accountInfo;
+                session.AccessibleVaults = accessibleVaults;
+            }
+
+            // 2. Check if the vault ID is valid and the vault exists
+            if (!session.AccountInfo.Vaults.Any(x => x.Id == vaultId))
+                return NoItem.NotFound;
+
+            // 3. Even if the vault is there, it might not be accessible
+            if (!session.AccessibleVaults.Any(x => x.Id == vaultId))
+                return NoItem.Inaccessible;
+
+            // 4. Download the item
+            var oneOf3 = GetVaultItem(itemId, vaultId, session.Keychain, session.Key, session.Rest);
+
+            // 5. Check if the item is not available
+            if (oneOf3.TryPickT2(out var noItem, out var oneOf2))
+                return noItem;
 
-            // Attempt to fetch everything necessary to decrypt the item
-            var accountInfo = GetAccountInfo(session.Key, session.Rest);
-            var keysets = GetKeysets(session.Key, session.Rest);
-            DecryptKeysets(keysets.Keysets, session.Credentials, session.Keychain);
-            GetAccessibleVaults(accountInfo, session.Keychain).FirstOrDefault(x => x.Id == vaultId)?.DecryptKeyIntoKeychain();
+            // 6. It's either an account or a SSH key
+            var item = oneOf2.Match<VaultItem>(a => a, k => k);
 
             if (CanDecrypt(item))
                 return oneOf3;
 
-            throw new InternalErrorException("Failed to fetch the keys to decrypt the item");
+            return NoItem.Inaccessible;
 
             bool CanDecrypt(VaultItem i) => session.Keychain.CanDecrypt(i.EncryptedOverview) && session.Keychain.CanDecrypt(i.EncryptedDetails);
         }
@@ -647,6 +670,7 @@ internal static string SubmitSecondFactorResult(SecondFactorKind factor, SecondF
             }
         }
 
+        // TODO: Rename to RequestAccountInfo
         internal static R.AccountInfo GetAccountInfo(AesKey sessionKey, RestClient rest)
         {
             return GetEncryptedJson<R.AccountInfo>(
@@ -656,11 +680,13 @@ internal static R.AccountInfo GetAccountInfo(AesKey sessionKey, RestClient rest)
             );
         }
 
+        // TODO: Rename to RequestKeysets
         internal static R.KeysetsInfo GetKeysets(AesKey sessionKey, RestClient rest)
         {
             return GetEncryptedJson<R.KeysetsInfo>("v1/account/keysets", sessionKey, rest);
         }
 
+        // TODO: We don't really need IEnumerable here
         internal static IEnumerable<VaultInfo> GetAccessibleVaults(R.AccountInfo accountInfo, Keychain keychain)
         {
             return from vault in accountInfo.Vaults
@@ -736,15 +762,18 @@ internal static OneOf<Account, SshKey, NoItem> GetVaultItem(
             RestClient rest
         )
         {
+            // TODO: Make a request to var response = rest.Get<R.Encrypted>($"v1/vault/{vaultId}/"); to check if the vault exists!
             var response = rest.Get<R.Encrypted>($"v1/vault/{vaultId}/item/{itemId}");
             if (response.IsSuccessful)
                 return ConvertVaultItem(keychain, DecryptResponse<R.SingleVaultItem>(response.Data, sessionKey).Item);
 
-            // Special case: the item not found
-            if (response.StatusCode == System.Net.HttpStatusCode.BadRequest && response.Content.Trim() == "{}")
-                return NoItem.NotFound;
-
-            throw MakeError(response);
+            var error = MakeError(response);
+            return error switch
+            {
+                // Special case: the item not found
+                NotFoundException => NoItem.NotFound,
+                _ => throw error,
+            };
         }
 
         // TODO: Rename to RequestVaultAccounts? It should clearer from the name that it's a slow operation.
@@ -844,9 +873,11 @@ internal static AesKey DeriveMasterKey(string algorithm, int iterations, byte[]
         }
 
         //
-        // HTTP
+        // Error handling
         //
 
+        internal class NotFoundException(string message) : BaseException(message);
+
         internal static BaseException MakeError(RestResponse<string> response)
         {
             if (response.IsNetworkError)
@@ -869,6 +900,8 @@ internal static BaseException ParseServerError(string response)
                 {
                     case 102:
                         return new BadCredentialsException("Username, password or account key is incorrect");
+                    case 117:
+                        return new NotFoundException($"The requested item not found: '{error.Message}'");
                     default:
                         return new InternalErrorException($"The server responded with the error code {error.Code} and the message '{error.Message}'");
                 }
@@ -892,6 +925,10 @@ internal static BaseException ParseServerError(string response)
             return null;
         }
 
+        //
+        // Network
+        //
+
         internal static T GetEncryptedJson<T>(string endpoint, AesKey sessionKey, RestClient rest)
         {
             var response = rest.Get<R.Encrypted>(endpoint);

src/OnePassword/NoItem.cs

@@ -8,4 +8,5 @@ public enum NoItem
     NotFound,
     Deleted,
     UnsupportedType,
+    Inaccessible,
 }

src/OnePassword/Session.cs

@@ -1,7 +1,10 @@
 // Copyright (C) Dmitry Yakimenko (detunized@gmail.com).
 // Licensed under the terms of the MIT license. See LICENCE for details.
 
+#nullable enable
+
 using PasswordManagerAccess.Common;
+using R = PasswordManagerAccess.OnePassword.Response;
 
 namespace PasswordManagerAccess.OnePassword
 {
@@ -15,6 +18,10 @@ public class Session
         internal readonly RestClient Rest;
         internal readonly IRestTransport Transport;
 
+        // Cache
+        internal R.AccountInfo? AccountInfo { get; set; }
+        internal VaultInfo[]? AccessibleVaults { get; set; }
+
         internal Session(Credentials credentials, Keychain keychain, AesKey key, RestClient rest, IRestTransport transport)
         {
             Credentials = credentials;

test/OnePassword/ClientTest.cs

@@ -24,6 +24,83 @@ namespace PasswordManagerAccess.Test.OnePassword
 {
     public class ClientTest : TestBase
     {
+        //
+        // Public methods
+        //
+
+        [Fact]
+        public void GetItem_returns_account()
+        {
+            // Arrange
+            var transport = new RestFlow()
+                .Get(EncryptFixture("get-vault-item-response"))
+                .ExpectUrl("1password.com/api/v1/vault/ru74fjxlkipzzctorwj4icrj2a/item/wm3uxq4xsmb4mghxw6o3s7zrem");
+            var rest = transport.ToRestClient(ApiUrl);
+            var keychain = new Keychain(
+                new AesKey("x4ouqoqyhcnqojrgubso4hsdga", "ce92c6d1af345c645211ad49692b22338d128d974e3b6718c868e02776c873a9".DecodeHex())
+            );
+            var accountInfo = JsonConvert.DeserializeObject<R.AccountInfo>(GetFixture("get-account-info-response"));
+            var vault = accountInfo.Vaults.First(x => x.Id == "ru74fjxlkipzzctorwj4icrj2a");
+            var vaultInfo = new VaultInfo(vault.Id, Encrypted.Parse(vault.Attributes), Encrypted.Parse(vault.Access[0].EncryptedKey), keychain);
+            var session = new Session(TestData.Credentials, keychain, TestData.SessionKey, rest, transport)
+            {
+                AccountInfo = accountInfo,
+                AccessibleVaults = [vaultInfo],
+            };
+
+            // Act
+            var result = Client.GetItem("wm3uxq4xsmb4mghxw6o3s7zrem", "ru74fjxlkipzzctorwj4icrj2a", session);
+
+            // Assert
+            result.TryPickT0(out var account, out _).ShouldBeTrue();
+            account.Id.ShouldBe("wm3uxq4xsmb4mghxw6o3s7zrem");
+        }
+
+        [Fact]
+        public void GetItem_returns_NoItem_when_not_found()
+        {
+            // Arrange
+            var transport = new RestFlow()
+                .Get(GetFixture("get-vault-item-not-found-response"), HttpStatusCode.NotFound)
+                .ExpectUrl("1password.com/api/v1/vault/ru74fjxlkipzzctorwj4icrj2a/item/nonexistent");
+            var rest = transport.ToRestClient(ApiUrl);
+            var keychain = new Keychain(
+                new AesKey("x4ouqoqyhcnqojrgubso4hsdga", "ce92c6d1af345c645211ad49692b22338d128d974e3b6718c868e02776c873a9".DecodeHex())
+            );
+            var accountInfo = JsonConvert.DeserializeObject<R.AccountInfo>(GetFixture("get-account-info-response"));
+            var vault = accountInfo.Vaults.First(x => x.Id == "ru74fjxlkipzzctorwj4icrj2a");
+            var vaultInfo = new VaultInfo(vault.Id, Encrypted.Parse(vault.Attributes), Encrypted.Parse(vault.Access[0].EncryptedKey), keychain);
+            var session = new Session(TestData.Credentials, keychain, TestData.SessionKey, rest, transport)
+            {
+                AccountInfo = accountInfo,
+                AccessibleVaults = [vaultInfo],
+            };
+
+            // Act
+            var result = Client.GetItem("nonexistent", "ru74fjxlkipzzctorwj4icrj2a", session);
+
+            // Assert
+            result.TryPickT2(out var noItem, out _).ShouldBeTrue();
+            noItem.ShouldBe(NoItem.NotFound);
+        }
+
+        [Fact]
+        public void ListAllVaults_returns_vaults()
+        {
+            // Arrange
+            var flow = new RestFlow().Get(EncryptFixture("get-account-info-response")).Get(EncryptFixture("get-keysets-response"));
+
+            // Act
+            var vaults = Client.ListAllVaults(Credentials, new Keychain(), TestData.SessionKey, flow);
+
+            // Assert
+            vaults.ShouldNotBeEmpty();
+        }
+
+        //
+        // Internal methods
+        //
+
         [Fact]
         public void ParseServiceAccountToken_returns_parsed_token()
         {
@@ -56,19 +133,6 @@ public void ParseServiceAccountToken_throws_on_invalid_token(string token)
             ex.Message.ShouldStartWith("Invalid service account token");
         }
 
-        [Fact]
-        public void ListAllVaults_returns_vaults()
-        {
-            // Arrange
-            var flow = new RestFlow().Get(EncryptFixture("get-account-info-response")).Get(EncryptFixture("get-keysets-response"));
-
-            // Act
-            var vaults = Client.ListAllVaults(Credentials, new Keychain(), TestData.SessionKey, flow);
-
-            // Assert
-            vaults.ShouldNotBeEmpty();
-        }
-
         [Fact]
         public void StartNewSession_returns_session_on_ok()
         {
@@ -479,7 +543,7 @@ public void GetKeysets_makes_GET_request_to_specific_url()
         }
 
         [Fact]
-        public void GetVaultAccounts_returns_accounts()
+        public void GetVaultItems_returns_accounts()
         {
             // Arrange
             var flow = new RestFlow().Get(EncryptFixture("get-vault-accounts-ru74-response"));
@@ -495,7 +559,7 @@ public void GetVaultAccounts_returns_accounts()
         }
 
         [Fact]
-        public void GetVaultAccounts_returns_ssh_keys()
+        public void GetVaultItems_returns_ssh_keys()
         {
             // Arrange
             var flow = new RestFlow().Get(EncryptFixture("get-vault-accounts-ixsi-response"));
@@ -520,7 +584,7 @@ public void GetVaultAccounts_returns_ssh_keys()
         }
 
         [Fact]
-        public void GetVaultAccounts_returns_converts_ssh_keys()
+        public void GetVaultItems_returns_converts_ssh_keys()
         {
             // Arrange
             var flow = new RestFlow().Get(EncryptFixture("get-vault-accounts-saiw-response"));
@@ -632,7 +696,7 @@ public void GetVaultAccounts_returns_converts_ssh_keys()
         }
 
         [Fact]
-        public void GetVaultAccounts_with_no_items_work()
+        public void GetVaultItems_with_no_items_work()
         {
             // Arrange
             var flow = new RestFlow().Get(EncryptFixture("get-vault-with-no-items-response"));
@@ -648,7 +712,7 @@ public void GetVaultAccounts_with_no_items_work()
         }
 
         [Fact]
-        public void GetVaultAccounts_returns_server_secrets()
+        public void GetVaultItems_returns_server_secrets()
         {
             // Arrange
             var flow = new RestFlow().Get(EncryptFixture("get-vault-with-server-secrets-response"));
@@ -664,7 +728,7 @@ public void GetVaultAccounts_returns_server_secrets()
         }
 
         [Fact]
-        public void GetVaultAccounts_makes_GET_request_to_specific_url()
+        public void GetVaultItems_makes_GET_request_to_specific_url()
         {
             // Arrange
             var flow = new RestFlow()
@@ -680,7 +744,7 @@ public void GetVaultAccounts_makes_GET_request_to_specific_url()
         }
 
         [Fact]
-        public void GetVaultAccounts_with_multiple_batches_returns_all_accounts()
+        public void GetVaultItems_with_multiple_batches_returns_all_accounts()
         {
             // Arrange
             var flow = new RestFlow()

test/OnePassword/Fixtures/get-vault-item-not-found-response.json

@@ -0,0 +1,5 @@
+{
+    "errorCode": 117,
+    "errorMessage": "Not found.",
+    "requestId": 20988701
+}

test/OnePassword/Fixtures/get-vault-item-response.json

@@ -0,0 +1,28 @@
+{
+    "contentVersion": 3,
+    "item": {
+        "uuid": "wm3uxq4xsmb4mghxw6o3s7zrem",
+        "templateUuid": "001",
+        "trashed": "N",
+        "createdAt": "2016-08-04T13:15:10Z",
+        "updatedAt": "2016-08-04T13:16:07Z",
+        "changerUuid": "DFM4DHQMQ5AS5M6UHSD6PNV774",
+        "packageUuid": "",
+        "itemVersion": 2,
+        "encryptedBy": "x4ouqoqyhcnqojrgubso4hsdga",
+        "encOverview": {
+            "cty": "b5+jwk+json",
+            "data": "9eH5PkEAnOA5QDds7BrUuwL3vo2abF1SSzwwVXwjnA_4zacK-qBCoK7flbzG-Pi_PIa9uxk3XkiJ4QavGD3FBJpdW9xxcvS5jadnyhmOfUNt7AKPLcg96hh0SLX9GdMnOGNXW_SW_UdBV4N-ux9wHv3dEf6gUw5emAYmucDcyGt4bvxNtXeFTFnZhuAB-lGA9l5xzjc",
+            "enc": "A256GCM",
+            "iv": "tgpqo7Cz06z2ka-W",
+            "kid": "x4ouqoqyhcnqojrgubso4hsdga"
+        },
+        "encDetails": {
+            "cty": "b5+jwk+json",
+            "data": "d-EtBOtn5hJWRX-IRVZc03axWv_EMDZtpCgFJEzP0qAHoCv8JTZJ00oWWDCINR2zFoHg5pks8ZvEMV1nquPwmN2hQxrB9mS8nK5wfWylgKms56VsdwD3P8SIZZcUOywX79oVN7VEu5C3fqBdeAiVvG_76ziHPFryApSbk9I34dIow6uZ0ldxGd5RPVaYRPkkZ1tf3_Nmgnm2n11JxEf0fSgj9Bc554hIMWLn7x3xYJbBgot_fYHqGP0UG7oBG0mXym1Lm_6lZFopQBlgIB0",
+            "enc": "A256GCM",
+            "iv": "RY7AOcB6_bTUwEwN",
+            "kid": "x4ouqoqyhcnqojrgubso4hsdga"
+        }
+    }
+}