Make ProxyUtil.IsAccessible(MethodBase) take into account declaring type's accessibility

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Castle Core Changelog
 
+## Unreleased
+
+Bugfixes:
+- Make ProxyUtil.IsAccessible(MethodBase) take into account declaring type's accessibility so it doesn't report false negatives for e.g. public methods in inaccessible classes. (@stakx, #289)
+
 ## 4.1.1 (2017-07-12)
 
 Bugfixes:

src/Castle.Core.Tests/DynamicProxy.Tests/ProxyUtilTestCase.cs

@@ -14,6 +14,7 @@
 
 namespace Castle.DynamicProxy.Tests
 {
+	using System;
 	using System.Reflection;
 
 	using Castle.DynamicProxy;
@@ -23,76 +24,77 @@ namespace Castle.DynamicProxy.Tests
 	[TestFixture]
 	public class ProxyUtilTestCase
 	{
-		[TestCaseSource("AccessibleMethods")]
-		public void IsAccessible_Accessible_Method_Returns_True(string methodName)
+		[TestCaseSource(nameof(AccessibleMethods))]
+		public void IsAccessible_Accessible_Method_Returns_True(MethodBase method)
 		{
-			Assert.IsTrue(ProxyUtil.IsAccessible(GetMethod<TestClass>(methodName)));
+			Assert.IsTrue(ProxyUtil.IsAccessible(method));
 		}
 
-		[TestCaseSource("InaccessibleMethods")]
-		public void IsAccessible_Inaccessible_Method_ReturnsFalse(string methodName)
+		[TestCaseSource(nameof(InaccessibleMethods))]
+		public void IsAccessible_Inaccessible_Method_Returns_False(MethodBase method)
 		{
-			Assert.IsFalse(ProxyUtil.IsAccessible(GetMethod<TestClass>(methodName)));
+			Assert.IsFalse(ProxyUtil.IsAccessible(method));
 		}
 
-		[TestCaseSource("AccessibleMethods")]
-		public void IsAccessibleWithReason_Accessible_Method_Returns_True(string methodName)
+		[TestCaseSource(nameof(AccessibleMethods))]
+		public void IsAccessibleWithReason_Accessible_Method_Returns_True(MethodBase method)
 		{
 			string reason;
-			Assert.IsTrue(ProxyUtil.IsAccessible(GetMethod<TestClass>(methodName), out reason));
+			Assert.IsTrue(ProxyUtil.IsAccessible(method, out reason));
 		}
 
-		[TestCaseSource("AccessibleMethods")]
-		public void IsAccessibleWithReason_Accessible_Method_Does_Not_Populate_ReasonMethodIsNotAccessible(string methodName)
+		[TestCaseSource(nameof(AccessibleMethods))]
+		public void IsAccessibleWithReason_Accessible_Method_Does_Not_Populate_ReasonMethodIsNotAccessible(MethodBase method)
 		{
 			string reason;
-			ProxyUtil.IsAccessible(GetMethod<TestClass>(methodName), out reason);
+			ProxyUtil.IsAccessible(method, out reason);
 
 			Assert.IsNull(reason);
 		}
 
-		[TestCaseSource("InaccessibleMethods")]
-		public void IsAccessibleWithReason_Inaccessible_Method_ReturnsFalse(string methodName)
+		[TestCaseSource(nameof(InaccessibleMethods))]
+		public void IsAccessibleWithReason_Inaccessible_Method_Returns_False(MethodBase method)
 		{
 			string reason;
-			Assert.IsFalse(ProxyUtil.IsAccessible(GetMethod<TestClass>(methodName), out reason));
+			Assert.IsFalse(ProxyUtil.IsAccessible(method, out reason));
 		}
 
-		[TestCaseSource("InaccessibleMethods")]
-		public void IsAccessibleWithReason_Inaccessible_Method_Populates_ReasonMethodIsNotAccessible(string methodName)
+		[TestCaseSource(nameof(InaccessibleMethods))]
+		public void IsAccessibleWithReason_Inaccessible_Method_Populates_ReasonMethodIsNotAccessible(MethodBase method)
 		{
 			string reason;
-			ProxyUtil.IsAccessible(GetMethod<TestClass>(methodName), out reason);
+			ProxyUtil.IsAccessible(method, out reason);
 
 #if FEATURE_GET_REFERENCED_ASSEMBLIES
-			var expectedReason = "Can not create proxy for method Void APrivateMethod() because it is not accessible. Make it public, or internal and mark your assembly with [assembly: InternalsVisibleTo(InternalsVisible.ToDynamicProxyGenAssembly2)] attribute, because assembly Castle.Core.Tests is strong-named.";
+			var expectedReason = "Can not create proxy for method Void " + method.Name + "() because it or its declaring type is not accessible. Make it public, or internal and mark your assembly with [assembly: InternalsVisibleTo(InternalsVisible.ToDynamicProxyGenAssembly2)] attribute, because assembly Castle.Core.Tests is strong-named.";
 #else
-			var expectedReason = "Can not create proxy for method Void APrivateMethod() because it is not accessible. Make it public, or internal and mark your assembly with [assembly: InternalsVisibleTo(\"DynamicProxyGenAssembly2, PublicKey=0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7\")] attribute, because assembly Castle.Core.Tests is strong-named.";
+			var expectedReason = "Can not create proxy for method Void " + method.Name + "() because it or its declaring type is not accessible. Make it public, or internal and mark your assembly with [assembly: InternalsVisibleTo(\"DynamicProxyGenAssembly2, PublicKey=0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7\")] attribute, because assembly Castle.Core.Tests is strong-named.";
 #endif
 
 			Assert.AreEqual(expectedReason, reason);
 		}
 
-		private static MethodInfo GetMethod<T>(string name)
+		private static MethodInfo GetMethod(Type declaringType, string name)
 		{
-			return typeof(T).GetMethod(name,
+			return declaringType.GetMethod(name,
 				BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
 		}
 
-		public static readonly object[] AccessibleMethods =
+		public static readonly MethodBase[] AccessibleMethods =
 		{
-			new object[] { "APublicMethod" },
-			new object[] { "AProtectedMethod" },
-			new object[] { "AnInternalMethod" }, // because our internals are visible to DynamicProxy2 
-			new object[] { "AProtectedInternalMethod" }
+			GetMethod(typeof(PublicTestClass), "APublicMethod"),
+			GetMethod(typeof(PublicTestClass), "AProtectedMethod"),
+			GetMethod(typeof(PublicTestClass), "AnInternalMethod"), // because our internals are visible to DynamicProxy2
+			GetMethod(typeof(PublicTestClass), "AProtectedInternalMethod"),
 		};
 
-		public static readonly object[] InaccessibleMethods =
+		public static readonly MethodBase[] InaccessibleMethods =
 		{
-			new object[] { "APrivateMethod" },
+			GetMethod(typeof(PublicTestClass), "APrivateMethod"),
+			GetMethod(typeof(PrivateTestClass), "APublicMethod"),
 		};
 
-		public class TestClass
+		public class PublicTestClass
 		{
 			public void APublicMethod()
 			{
@@ -114,5 +116,12 @@ private void APrivateMethod()
 			{
 			}
 		}
+
+		private class PrivateTestClass
+		{
+			public void APublicMethod()
+			{
+			}
+		}
 	}
-}
+}

src/Castle.Core/DynamicProxy/Contributors/ClassMembersCollector.cs

@@ -18,7 +18,6 @@ namespace Castle.DynamicProxy.Contributors
 	using System.Reflection;
 
 	using Castle.DynamicProxy.Generators;
-	using Castle.DynamicProxy.Internal;
 
 	public class ClassMembersCollector : MembersCollector
 	{
@@ -29,7 +28,7 @@ public ClassMembersCollector(Type targetType)
 
 		protected override MetaMethod GetMethodToGenerate(MethodInfo method, IProxyGenerationHook hook, bool isStandalone)
 		{
-			if (method.IsAccessible() == false)
+			if (ProxyUtil.IsAccessibleMethod(method) == false)
 			{
 				return null;
 			}

src/Castle.Core/DynamicProxy/Contributors/InterfaceMembersCollector.cs

@@ -18,7 +18,6 @@ namespace Castle.DynamicProxy.Contributors
 	using System.Reflection;
 
 	using Castle.DynamicProxy.Generators;
-	using Castle.DynamicProxy.Internal;
 
 	public class InterfaceMembersCollector : MembersCollector
 	{
@@ -29,7 +28,7 @@ public InterfaceMembersCollector(Type @interface)
 
 		protected override MetaMethod GetMethodToGenerate(MethodInfo method, IProxyGenerationHook hook, bool isStandalone)
 		{
-			if (method.IsAccessible() == false)
+			if (ProxyUtil.IsAccessibleMethod(method) == false)
 			{
 				return null;
 			}

src/Castle.Core/DynamicProxy/Contributors/InterfaceMembersOnClassCollector.cs

@@ -18,7 +18,6 @@ namespace Castle.DynamicProxy.Contributors
 	using System.Reflection;
 
 	using Castle.DynamicProxy.Generators;
-	using Castle.DynamicProxy.Internal;
 
 	public class InterfaceMembersOnClassCollector : MembersCollector
 	{
@@ -33,7 +32,7 @@ public InterfaceMembersOnClassCollector(Type type, bool onlyProxyVirtual, Interf
 
 		protected override MetaMethod GetMethodToGenerate(MethodInfo method, IProxyGenerationHook hook, bool isStandalone)
 		{
-			if (method.IsAccessible() == false)
+			if (ProxyUtil.IsAccessibleMethod(method) == false)
 			{
 				return null;
 			}

src/Castle.Core/DynamicProxy/Contributors/MembersCollector.cs

@@ -21,7 +21,6 @@ namespace Castle.DynamicProxy.Contributors
 
 	using Castle.Core.Logging;
 	using Castle.DynamicProxy.Generators;
-	using Castle.DynamicProxy.Generators.Emitters;
 	using Castle.DynamicProxy.Internal;
 
 	public abstract class MembersCollector
@@ -252,8 +251,8 @@ protected bool AcceptMethod(MethodInfo method, bool onlyVirtuals, IProxyGenerati
 
 		private static bool IsInternalAndNotVisibleToDynamicProxy(MethodInfo method)
 		{
-			return method.IsInternal() &&
-			       method.DeclaringType.GetTypeInfo().Assembly.IsInternalToDynamicProxy() == false;
+			return ProxyUtil.IsInternal(method) &&
+				   ProxyUtil.AreInternalsVisibleToDynamicProxy(method.DeclaringType.GetTypeInfo().Assembly) == false;
 		}
 	}
 }

src/Castle.Core/DynamicProxy/Contributors/WrappedClassMembersCollector.cs

@@ -37,7 +37,7 @@ public override void CollectMembersToProxy(IProxyGenerationHook hook)
 
 		protected override MetaMethod GetMethodToGenerate(MethodInfo method, IProxyGenerationHook hook, bool isStandalone)
 		{
-			if (method.IsAccessible() == false)
+			if (ProxyUtil.IsAccessibleMethod(method) == false)
 			{
 				return null;
 			}

src/Castle.Core/DynamicProxy/DefaultProxyBuilder.cs

@@ -22,9 +22,6 @@ namespace Castle.DynamicProxy
 	using Castle.Core.Internal;
 	using Castle.Core.Logging;
 	using Castle.DynamicProxy.Generators;
-	using Castle.DynamicProxy.Generators.Emitters;
-	using Castle.DynamicProxy.Internal;
-
 
 	/// <summary>
 	///   Default implementation of <see cref = "IProxyBuilder" /> interface producing in-memory proxy assemblies.
@@ -124,7 +121,7 @@ private void AssertValidTypeForTarget(Type type, Type target)
 				throw new GeneratorException(string.Format("Can not create proxy for type {0} because type {1} is an open generic type.",
 															target.GetBestName(), type.GetBestName()));
 			}
-			if (IsPublic(type) == false && IsAccessible(type) == false)
+			if (ProxyUtil.IsAccessibleType(type) == false)
 			{
 				throw new GeneratorException(ExceptionMessageBuilder.CreateMessageForInaccessibleType(type, target));
 			}
@@ -144,24 +141,5 @@ private void AssertValidTypes(IEnumerable<Type> targetTypes)
 				}
 			}
 		}
-
-		private bool IsAccessible(Type target)
-		{
-			return IsInternal(target) && target.GetTypeInfo().Assembly.IsInternalToDynamicProxy();
-		}
-
-		private bool IsPublic(Type target)
-		{
-			return target.GetTypeInfo().IsPublic || target.GetTypeInfo().IsNestedPublic;
-		}
-
-		private static bool IsInternal(Type target)
-		{
-			var isTargetNested = target.IsNested;
-			var isNestedAndInternal = isTargetNested && (target.GetTypeInfo().IsNestedAssembly || target.GetTypeInfo().IsNestedFamORAssem);
-			var isInternalNotNested = target.GetTypeInfo().IsVisible == false && isTargetNested == false;
-
-			return isInternalNotNested || isNestedAndInternal;
-		}
 	}
 }

src/Castle.Core/DynamicProxy/ExceptionMessageBuilder.cs

@@ -15,13 +15,56 @@
 namespace Castle.DynamicProxy
 {
 	using System;
+	using System.Linq;
 	using System.Reflection;
 
 	using Castle.Core.Internal;
-	using Castle.DynamicProxy.Internal;
+	using Castle.DynamicProxy.Generators.Emitters;
 
 	internal static class ExceptionMessageBuilder
 	{
+		/// <summary>
+		/// Provides instructions that a user could follow to make a type or method in <paramref name="targetAssembly"/>
+		/// visible to DynamicProxy.</summary>
+		/// <param name="targetAssembly">The assembly containing the type or method.</param>
+		/// <returns>Instructions that a user could follow to make a type or method visible to DynamicProxy.</returns>
+		internal static string CreateInstructionsToMakeVisible(Assembly targetAssembly)
+		{
+			string strongNamedOrNotIndicator = " not"; // assume not strong-named
+			string assemblyToBeVisibleTo = "\"DynamicProxyGenAssembly2\""; // appropriate for non-strong-named
+
+			if (targetAssembly.IsAssemblySigned())
+			{
+				strongNamedOrNotIndicator = "";
+				assemblyToBeVisibleTo = ReferencesCastleCore(targetAssembly)
+					? "InternalsVisible.ToDynamicProxyGenAssembly2"
+					: '"' + InternalsVisible.ToDynamicProxyGenAssembly2 + '"';
+			}
+
+			var instructionsFormat =
+				"Make it public, or internal and mark your assembly with " +
+				"[assembly: InternalsVisibleTo({0})] attribute, because assembly {1} " +
+				"is{2} strong-named.";
+
+			var instructions = String.Format(instructionsFormat,
+				assemblyToBeVisibleTo,
+				targetAssembly.GetName().Name,
+				strongNamedOrNotIndicator);
+			return instructions;
+
+			bool ReferencesCastleCore(Assembly ia)
+			{
+#if FEATURE_GET_REFERENCED_ASSEMBLIES
+				return ia.GetReferencedAssemblies()
+					.Any(r => r.FullName == Assembly.GetExecutingAssembly().FullName);
+#else
+				// .NET Core does not provide an API to do this, so we just fall back to the solution that will definitely work.
+				// After all it is just an exception message.
+				return false;
+#endif
+			}
+		}
+
 		/// <summary>
 		/// Creates a message to inform clients that a proxy couldn't be created due to reliance on an
 		/// inaccessible type (perhaps itself).
@@ -42,7 +85,7 @@ public static string CreateMessageForInaccessibleType(Type inaccessibleType, Typ
 				typeToProxy.GetBestName(),
 				inaccessibleTypeDescription);
 
-			var instructions = InternalsUtil.CreateInstructionsToMakeVisible(targetAssembly);
+			var instructions = CreateInstructionsToMakeVisible(targetAssembly);
 
 			return message + instructions;
 		}

src/Castle.Core/DynamicProxy/Generators/BaseProxyGenerator.cs

@@ -24,7 +24,6 @@ namespace Castle.DynamicProxy.Generators
 	using System.Xml.Serialization;
 #endif
 
-	using Castle.Core.Internal;
 	using Castle.Core.Logging;
 	using Castle.DynamicProxy.Contributors;
 	using Castle.DynamicProxy.Generators.Emitters;
@@ -426,7 +425,7 @@ private bool IsConstructorVisible(ConstructorInfo constructor)
 			return constructor.IsPublic ||
 				constructor.IsFamily ||
 				constructor.IsFamilyOrAssembly ||
-				(constructor.IsAssembly && InternalsUtil.IsInternalToDynamicProxy(constructor.DeclaringType.GetTypeInfo().Assembly));
+				(constructor.IsAssembly && ProxyUtil.AreInternalsVisibleToDynamicProxy(constructor.DeclaringType.GetTypeInfo().Assembly));
 		}
 
 		private bool OverridesEqualsAndGetHashCode(Type type)

src/Castle.Core/DynamicProxy/Generators/MetaMethod.cs

@@ -18,8 +18,6 @@ namespace Castle.DynamicProxy.Generators
 	using System.Diagnostics;
 	using System.Reflection;
 
-	using Castle.DynamicProxy.Internal;
-
 	[DebuggerDisplay("{Method}")]
 	public class MetaMethod : MetaTypeElement, IEquatable<MetaMethod>
 	{
@@ -123,8 +121,8 @@ private MethodAttributes ObtainAttributes()
 			{
 				attributes |= MethodAttributes.HideBySig;
 			}
-			if (InternalsUtil.IsInternal(methodInfo) &&
-				InternalsUtil.IsInternalToDynamicProxy(methodInfo.DeclaringType.GetTypeInfo().Assembly))
+			if (ProxyUtil.IsInternal(methodInfo) &&
+			    ProxyUtil.AreInternalsVisibleToDynamicProxy(methodInfo.DeclaringType.GetTypeInfo().Assembly))
 			{
 				attributes |= MethodAttributes.Assembly;
 			}