[d16-9] [monotouch-test] Rework big parts of KeyChainTest. Fixes #xam…

…arin/maccore@2365. (dotnet#10603)

* [monotouch-test] Rework big parts of KeyChainTest. Fixes #xamarin/maccore@2365.

'GenericPassword' keychain items are unique by their Service+Account
properties [1]. This means that changing the Label property will not create a
different 'GenericPassword', which has a few consequences:

* It's possible to filter (and try to delete) using the Label property.
* It's possible to try to delete a 'GenericPassword' item, and have that
  deletion attempt fail with 'no item found', and then subsequently trying to
  add the same item will fail with a DuplicateItem, because the deletion
  was filtered using the Label property.

The change I've made is to:

* Make the Label property much more descriptive, and unique per process. This
  makes it easier to figure out where things come from in the Keychain Access
  app.
* Make the Service property unique per process. This way these tests are
  parallel safe and they won't stomp on eachother.
* Keep the Account property the same (a constant value), so that it's easy to
  filter to just these items in the Keychain Access app.
* Remove the Label property from all queries, it doesn't matter anyway. The
  Label property is still set when adding items to the keychain.

Finally try to clean up after ourselves as good as possible. This way we don't
fill the keychain with test stuff. This involves removing certificates and
passwords we add to the keychain at the end of tests.

Fixes xamarin/maccore#2365.

[1]: https://stackoverflow.com/a/11672200/183422

* Adjust query for RecordTest.AuthenticationType as well to not include fields that don't determine uniqueness.

Also make the InternetPassword items we add to the keychain more descriptive and labelled helpfully.

* Add debug code.

* Revert "Add debug code."

This reverts commit a3edac8.

Co-authored-by: Rolf Bjarne Kvinge <[email protected]>

tests/monotouch-test/Security/KeyChainTest.cs

@@ -1,10 +1,12 @@
 // Copyright 2011, 2013 Xamarin Inc. All rights reserved
 
 using System;
+using System.Diagnostics;
 using System.Runtime.InteropServices;
 using System.IO;
 using System.Reflection;
 
+using CoreFoundation;
 using Foundation;
 using Security;
 using ObjCRuntime;
@@ -29,13 +31,22 @@ public void Add_Certificate ()
 #endif
 			NSData data = NSData.FromStream (certStream);
 
-			var rec = new SecRecord (SecKind.Certificate) {
-				Label = "MyCert"
+			var query = new SecRecord (SecKind.Certificate) {
+				Label = $"Internet Widgits Pty Ltd",
 			};
+			var rec = query.Clone ();
 			rec.SetValueRef (new SecCertificate (data));
 
-			var rc = SecKeyChain.Add (rec);
-			Assert.That (rc, Is.EqualTo (SecStatusCode.Success).Or.EqualTo (SecStatusCode.DuplicateItem), "Add_Certificate");
+			try {
+				// delete any existing certificates first.
+				SecKeyChain.Remove (query);
+				// add the new certificate
+				var rc = SecKeyChain.Add (rec);
+				Assert.That (rc, Is.EqualTo (SecStatusCode.Success), "Add_Certificate");
+			} finally {
+				// clean up after ourselves
+				SecKeyChain.Remove (query);
+			}
 		}
 
 #if !MONOMAC // No QueryAsConcreteType on Mac
@@ -91,34 +102,58 @@ public void SecItemAdd_Identity ()
 				Assert.That (code, expected);
 			}
 		}
-
-		static Guid GetID()
-		{           
-			Guid returnGuid = Guid.Empty;
+
+		string uniqueString;
+		string UniqueString {
+			get {
+				if (uniqueString == null)
+					uniqueString = $"{CFBundle.GetMain ().Identifier}-{GetType ().FullName}-{Process.GetCurrentProcess ().Id}";
+				return uniqueString;
+			}
+		}
+
+		string RecordLabel {
+			get {
+				return $"{UniqueString}-Label";
+			}
+		}
+
+		// The uniqueness of GenericPassword entries are based on Service+Account (only).
+		// Here we have a per-process + per-test unique Service value,
+		// and a constant Account value (which makes it easier to find all entries if need be).
+		string RecordService {
+			get {
+				return $"{UniqueString}-Service";
+			}
+		}
+
+		string RecordAccount {
+			get {
+				return $"XAMARIN_KEYCHAIN_ACCOUNT";
+			}
+		}
+
+		Guid GetID ()
+		{
 			SecStatusCode code;
 			SecRecord queryRec = new SecRecord (SecKind.GenericPassword) { 
-				Service = "KEYCHAIN_SERVICE", 
-				Label = "KEYCHAIN_SERVICE", 
-				Account = "KEYCHAIN_ACCOUNT" 
+				Service = RecordService,
+				Account = RecordAccount,
 			};
-			queryRec = SecKeyChain.QueryAsRecord (queryRec, out code);
+			var queryResponse = SecKeyChain.QueryAsRecord (queryRec, out code);
+			if (code == SecStatusCode.Success && queryResponse?.Generic != null)
+				return new Guid (NSString.FromData (queryResponse.Generic, NSStringEncoding.UTF8));
 
-			if (code == SecStatusCode.Success && queryRec != null && queryRec.Generic != null )
-			{
-				returnGuid = new Guid(NSString.FromData(queryRec.Generic, NSStringEncoding.UTF8));
-			}
-
-			return returnGuid;
+			return Guid.Empty;
 		}
 
 		[Test]
 		public void QueryAsData ()
 		{
 			SecStatusCode code;
 			SecRecord queryRec = new SecRecord (SecKind.GenericPassword) {
-				Service = "KEYCHAIN_SERVICE",
-				Label = "KEYCHAIN_SERVICE",
-				Account = "KEYCHAIN_ACCOUNT"
+				Service = RecordService,
+				Account = RecordAccount,
 			};
 			var data = SecKeyChain.QueryAsData (queryRec, true, out code);
 			if (code == SecStatusCode.Success && queryRec != null) {
@@ -131,29 +166,37 @@ public void QueryAsDataArray ()
 		{
 			SecStatusCode code;
 			SecRecord queryRec = new SecRecord (SecKind.GenericPassword) {
-				Service = "KEYCHAIN_SERVICE",
-				Label = "KEYCHAIN_SERVICE",
-				Account = "KEYCHAIN_ACCOUNT"
+				Service = RecordService,
+				Account = RecordAccount,
 			};
 			var data = SecKeyChain.QueryAsData (queryRec, true, 1, out code);
 			if (code == SecStatusCode.Success && queryRec != null) {
 				Assert.NotNull (data [0].Bytes);
 			}
 		}
-
-		static SecStatusCode SetID (Guid setID)
+
+		SecStatusCode RemoveID ()
+		{
+			var queryRec = new SecRecord (SecKind.GenericPassword) {
+				Service = RecordService,
+				Account = RecordAccount,
+			};
+			return SecKeyChain.Remove (queryRec);
+		}
+
+		SecStatusCode SetID (Guid setID)
 		{
-			var queryRec = new SecRecord (SecKind.GenericPassword) { 
-				Service = "KEYCHAIN_SERVICE", 
-				Label = "KEYCHAIN_SERVICE", 
-				Account = "KEYCHAIN_ACCOUNT" 
+			var queryRec = new SecRecord (SecKind.GenericPassword) {
+				Service = RecordService,
+				Account = RecordAccount,
 			};
 			var record = queryRec.Clone ();
 			record.Generic = NSData.FromString (Convert.ToString (setID), NSStringEncoding.UTF8);
 			record.Accessible = SecAccessible.Always;
+			record.Label = RecordLabel;
 			SecStatusCode code = SecKeyChain.Add (record);
 			if (code == SecStatusCode.DuplicateItem) {
-				code = SecKeyChain.Remove (queryRec);
+				code = RemoveID ();
 				if (code == SecStatusCode.Success)
 					code = SecKeyChain.Add (record);
 			}
@@ -167,8 +210,12 @@ public void CheckId ()
 			// test case from http://stackoverflow.com/questions/9481860/monotouch-cant-get-value-of-existing-keychain-item
 			// not a bug (no class lib fix) just a misuse of the API wrt status codes
 			Guid g = Guid.NewGuid ();
-			SetID (g);
-			Assert.That (g, Is.EqualTo (GetID ()), "same guid");
+			try {
+				Assert.That (SetID (g), Is.EqualTo (SecStatusCode.Success), "success");
+				Assert.That (GetID (), Is.EqualTo (g), "same guid");
+			} finally {
+				RemoveID ();
+			}
 		}
 	}
 }

tests/monotouch-test/Security/RecordTest.cs

@@ -1,6 +1,9 @@
 // Copyright 2014-2015 Xamarin Inc. All rights reserved
 
 using System;
+using System.Diagnostics;
+
+using CoreFoundation;
 using Foundation;
 using Security;
 #if MONOMAC
@@ -165,16 +168,22 @@ void AuthenticationType (SecAuthenticationType type)
 			SecKeyChain.Remove (rec); // it might already exists (or not)
 
 			rec = new SecRecord (SecKind.InternetPassword) {
-				Account = "AuthenticationType",
+				Account = $"{CFBundle.GetMain ().Identifier}-{GetType ().FullName}-{Process.GetCurrentProcess ().Id}",
 				ValueData = NSData.FromString ("Password"),
 				AuthenticationType = type,
 				Server = "www.xamarin.com"
 			};
 
 			Assert.That (SecKeyChain.Add (rec), Is.EqualTo (SecStatusCode.Success), "Add");
 
+			var query = new SecRecord (SecKind.InternetPassword) {
+				Account = rec.Account,
+				AuthenticationType = rec.AuthenticationType,
+				Server = rec.Server,
+			};
+
 			SecStatusCode code;
-			var match = SecKeyChain.QueryAsRecord (rec, out code);
+			var match = SecKeyChain.QueryAsRecord (query, out code);
 			Assert.That (code, Is.EqualTo (SecStatusCode.Success), "QueryAsRecord");
 
 			Assert.That (match.AuthenticationType, Is.EqualTo (type), "AuthenticationType");