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

stakx · 2017-07-19T14:37:51Z

This should fix #289.

I tried to follow @jonorossi's suggestions as closely as possible, but I might've done a few things too many here, or misunderstood something. If so, please let me know. See the commit notes for details. A few points regarding the code moves:

I moved everything from InternalsUtil to ProxyUtil. The former class now only contains "forwarding" methods which are hidden from IntelliSense, but not marked as [Obsolete]; none of them has an exact correspondence with a public method in ProxyUtil. (I chose not to make the moved methods public right away as that would've led to additional duplicate methods in the public API.)
Those three methods from DefaultProxyBuilder become a single method ProxyUtil.IsAccessibleType. (One is a helper method, and the other two only ever get called together. Combining them also allows optimizing away repeated calls to GetTypeInfo.)
Moving extension methods into the public /* non-static */ class ProxyUtil means they can no longer be extension methods. (I opted not to re-declare ProxyUtil as static right away.)

The preexisting tests for `ProxyUtil.IsAccessible(MethodBase)` only cover methods that are declared in a class that is accessible to DynamicProxy. Let's add one more test case where a public method is declared inside an inaccessible type. This will make some tests fail and thus reveal a bug.

This commit fixes the bug revealed in the preceding commit by making `ProxyUtil.IsAccessible(MethodBase)` take into account the declaring type's accessibility, as well. That method is currently a forward to the identically named method in `InternalsUtils` which also receives internals calls from DynamicProxy itself, so we need to clearly separate the public API and the internal API before we change any behaviour. This is done as follows: * The relationship between `ProxyUtil` and `InternalsUtil` is revers- ed, i.e. the actual implementations are moved to `ProxyUtil`, and `InternalsUtil`'s public methods are reduced to forwards. * In `ProxyUtil`, we now have internal methods (e.g. `IsAccessible- Method` and `IsAccessibleType`) which get called by DynamicProxy. We also have a separate public method group (`IsAccessible`) which makes use of the internal methods. Finally, we move in suitable preexisting code from `DefaultProxyBuil- der` that allows checking a declaring type's visibility.

jonorossi

@stakx great work, exactly what I was after and a little more which in this case is good. I do have a thought about where the implementation should go, see the comment.

Because you've touched a bunch of existing code it is fair game for me to add a few review comments to it as well, hope you don't mind make those changes too 😉 .

I definitely agree that those moved methods shouldn't be public, we'll make them public as required in the future, but we need to make the InternalsUtil ones Obsolete otherwise people won't stop using them.

jonorossi · 2017-07-21T12:01:53Z

src/Castle.Core/DynamicProxy/ProxyUtil.cs

+		/// visible to DynamicProxyGenAssembly2.</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 DynamicProxyGenAssembly2.</returns>
+		internal static string CreateInstructionsToMakeVisible(Assembly targetAssembly)


I think this method should go into ExceptionMessageBuilder instead, otherwise ProxyUtil is going to get big quickly.

The other option would be to keep InternalsUtil which can hold the implementation and ProxyUtil just forwards through, we could keep the obsolete public methods at the top of the file for now forwarding to internal methods below. Thoughts?

Agreed. I think ExceptionMessageBuilder' would be a good and fitting new home for CreateInstructionsToMakeVisible.

(Given its somewhat vague name, InternalsUtil could easily become a Kitchen Sink Of Lost Methods, and it would perhaps be better to stop putting new stuff in there and get rid of it as soon as is practical. Especially since other utility classes already have internal things in them.)

jonorossi · 2017-07-21T12:02:34Z

src/Castle.Core/DynamicProxy/ProxyUtil.cs

+
+			string GetAssemblyName(Assembly ta)
+			{
+				return ta.GetName().Name;


I'd just inline this, the method was only used once in the old code.

jonorossi · 2017-07-21T12:03:01Z

src/Castle.Core/DynamicProxy/ProxyUtil.cs

+#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;


Indentation.

jonorossi · 2017-07-21T12:05:00Z

src/Castle.Core/DynamicProxy/ProxyUtil.cs

+				strongNamedOrNotIndicator = "";
+				assemblyToBeVisibleTo = ReferencesCastleCore(targetAssembly)
+					? "InternalsVisible.ToDynamicProxyGenAssembly2"
+					: '"' + InternalsVisible.ToDynamicProxyGenAssembly2 + '"';


I know this was in the old code exactly as is, however do we actually need this, what does the message look like with the extra double quotes?

I'd submit that we do need it. Okay, maybe not "need". The bit after ? provides a convenience mechanism for assemblies that reference Castle.Core. They can just add

[assembly: InternalsVisibleTo("InternalsVisible.ToDynamicProxyGenAssembly2")]

instead of providing the full reference to the assembly themselves. The bit after the : gives a message like

Can not create proxy for type FluentAssertions.Common.NullReflector because it is not accessible. Make it public, or internal and mark your assembly with [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2, PublicKey=0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7")] attribute, because assembly FluentAssertions.Core is strong-named.

a

Thanks @blairconrad, I think I was starring at that code too long, completely missed that it was referencing that field with the strong name key and not just a constant string literal.

All good, it can stay as is.

jonorossi · 2017-07-21T12:08:22Z

src/Castle.Core/DynamicProxy/ProxyUtil.cs

+		/// <returns><c>true</c> if the method is accessible to DynamicProxy, <c>false</c> otherwise.</returns>
+		internal static bool IsAccessibleMethod(MethodBase method)
+		{
+			// Accessibility supported by the full framework and CoreCLR


This comment can be deleted, "CoreCLR" in this context referred to Silverlight's "CoreCLR".

jonorossi · 2017-07-21T12:19:09Z

src/Castle.Core/DynamicProxy/ProxyUtil.cs

+				return true;
+			}
+
+			if (AreInternalsVisibleToDynamicProxy(method.DeclaringType.GetTypeInfo().Assembly) && method.IsAssembly)


The method.IsAssembly check should be first as it is much less expensive.

jonorossi · 2017-07-21T12:23:46Z

src/Castle.Core/DynamicProxy/ProxyUtil.cs

+
+			bool VisibleToDynamicProxy(InternalsVisibleToAttribute attribute)
+			{
+				return attribute.AssemblyName.Contains(ModuleScope.DEFAULT_ASSEMBLY_NAME);


I know this was its own method before, but this should just be an inline lambda.

jonorossi · 2017-07-21T12:25:09Z

src/Castle.Core/DynamicProxy/ProxyUtil.cs

+		///   Determines whether this assembly has internals visible to dynamic proxy.
+		/// </summary>
+		/// <param name = "asm">The assembly to inspect.</param>
+		internal static bool AreInternalsVisibleToDynamicProxy(Assembly asm)


Could you move this internal function further down, with the methods for accessibility.

jonorossi · 2017-07-21T12:26:28Z

src/Castle.Core/DynamicProxy/ProxyUtil.cs

 #if FEATURE_REMOTING
 	using System.Runtime.Remoting;
 #endif

+	using Castle.Core.Internal;
+	using Castle.DynamicProxy.Generators.Emitters;
+
 	public class ProxyUtil


Let's go ahead and make this static like it should have been, however don't change the functions back to extension methods.

jonorossi · 2017-07-21T12:29:25Z

src/Castle.Core/DynamicProxy/Internal/InternalsUtil.cs

 		/// <summary>
 		///   Determines whether the specified method is internal.
 		/// </summary>
 		/// <param name = "method">The method.</param>
 		/// <returns>
 		///   <c>true</c> if the specified method is internal; otherwise, <c>false</c>.
 		/// </returns>
+		[EditorBrowsable(EditorBrowsableState.Never)]


We should just mark these obsolete, your call if you want to leave EditorBrowsable on there too.

Added the [Obsolete] attributes. I left [EditorBrowsable] in place though, hope you really don't mind. It could in theory even be put on the whole class, but perhaps you're still going to use that class in the future so I didn't do this.

I left [EditorBrowsable] in place though, hope you really don't mind.

Nope, it's all good.

(This commit includes a minor bug fix regarding visibility checks for methods that are declared with `FamilyAndAssembly` accessibility, which requires an additional check for `[InternalsVisibleTo]`.)

stakx · 2017-07-21T18:02:35Z

CHANGELOG.md

+
+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)
+


Did you want the following mentioned as well?

The ProxyUtil class having become static. (Possibly a breaking change, even though it's very unlikely.)

Members of InternalsUtil now being obsolete.

If so, under what heading do you usually list non-breaking changes? Just Changes: above Bugfixes:? (I didn't see any previous example.)

I wouldn't, if anyone was inheriting this class, then bad luck 😉

It could be breaking if someone re-compiles and has warnings as errors, however I think we just mention it when we actually remove them so we don't bloat the changelog.

If so, under what heading do you usually list non-breaking changes?

I think I've used "Enhancements" in one of the changelogs but that isn't applicable here obviously. Something to decide when it comes up.

stakx · 2017-07-21T18:11:15Z

src/Castle.Core/DynamicProxy/ProxyUtil.cs

+			if (method.IsAssembly || method.IsFamilyAndAssembly)
+			{
+				return AreInternalsVisibleToDynamicProxy(method.DeclaringType.GetTypeInfo().Assembly);
+			}


Instead of merging the check for method.IsFamilyAndAssembly with the first if block, I merged it with the latter (as per your comment).

jonorossi

All good, let's get this merged!

jonorossi · 2017-07-25T12:45:30Z

src/Castle.Core/DynamicProxy/Internal/InternalsUtil.cs

 		/// <summary>
 		///   Determines whether the specified method is internal.
 		/// </summary>
 		/// <param name = "method">The method.</param>
 		/// <returns>
 		///   <c>true</c> if the specified method is internal; otherwise, <c>false</c>.
 		/// </returns>
+		[EditorBrowsable(EditorBrowsableState.Never)]


I left [EditorBrowsable] in place though, hope you really don't mind.

Nope, it's all good.

jonorossi · 2017-07-25T12:53:17Z

CHANGELOG.md

+
+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)
+


I wouldn't, if anyone was inheriting this class, then bad luck 😉

It could be breaking if someone re-compiles and has warnings as errors, however I think we just mention it when we actually remove them so we don't bloat the changelog.

If so, under what heading do you usually list non-breaking changes?

I think I've used "Enhancements" in one of the changelogs but that isn't applicable here obviously. Something to decide when it comes up.

stakx force-pushed the proxyutil-isaccessible branch from e95210a to 0115d10 Compare July 19, 2017 14:42

stakx added 2 commits July 19, 2017 16:48

Optimize away redundant calls to GetTypeInfo

e1ce0f8

stakx force-pushed the proxyutil-isaccessible branch from 0115d10 to e1ce0f8 Compare July 19, 2017 14:48

jonorossi requested changes Jul 21, 2017

View reviewed changes

stakx added 2 commits July 21, 2017 19:17

Simplify code, move methods to more fitting places

f391fd0

(This commit includes a minor bug fix regarding visibility checks for methods that are declared with `FamilyAndAssembly` accessibility, which requires an additional check for `[InternalsVisibleTo]`.)

Update CHANGELOG.md

2cde9f5

stakx commented Jul 21, 2017

View reviewed changes

jonorossi approved these changes Jul 25, 2017

View reviewed changes

jonorossi merged commit 29e5eb2 into castleproject:master Jul 25, 2017

jonorossi added this to the vNext milestone Jul 25, 2017

stakx deleted the proxyutil-isaccessible branch July 25, 2017 13:12

stakx mentioned this pull request Sep 30, 2017

System.MissingMethodException Method not found: 'Boolean Castle.DynamicProxy.ProxyUtil.IsAccessible()' #308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stakx commented Jul 19, 2017 •

edited

Loading

jonorossi left a comment

jonorossi Jul 21, 2017

stakx Jul 21, 2017 •

edited

Loading

jonorossi Jul 21, 2017

jonorossi Jul 21, 2017

jonorossi Jul 21, 2017

blairconrad Jul 21, 2017

jonorossi Jul 21, 2017

jonorossi Jul 21, 2017

jonorossi Jul 21, 2017

jonorossi Jul 21, 2017

jonorossi Jul 21, 2017

jonorossi Jul 21, 2017

jonorossi Jul 21, 2017

stakx Jul 21, 2017 •

edited

Loading

jonorossi Jul 25, 2017

stakx Jul 21, 2017 •

edited

Loading

jonorossi Jul 25, 2017

stakx Jul 21, 2017

jonorossi left a comment

jonorossi Jul 25, 2017

jonorossi Jul 25, 2017


		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)

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

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

Conversation

stakx commented Jul 19, 2017 • edited Loading

jonorossi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonorossi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

stakx commented Jul 19, 2017 •

edited

Loading

stakx Jul 21, 2017 •

edited

Loading

stakx Jul 21, 2017 •

edited

Loading

stakx Jul 21, 2017 •

edited

Loading