Add new analyzer for ICorsHeaderAppender/CorsHeaderAppender usage #326
Conversation
…rAppender/CorsHeaderAppender
| /// <summary> | ||
| /// We never want to allow manual instantiation of this object, so no whitelist here. | ||
| /// </summary> | ||
| private static void PreventManualInstantiation( |
There was a problem hiding this comment.
I'm not sure if this is overkill. I figured for safety's sake I would include it, but if it's not worth it I can strip it out.
| } | ||
|
|
||
| foreach( var parameter in constructor.ParameterList.Parameters ) { | ||
| INamedTypeSymbol baseType = context.SemanticModel.GetTypeInfo( parameter.Type ).Type as INamedTypeSymbol; |
There was a problem hiding this comment.
baseType is a bit confusing (because of base classes) maybe paramType?
| return; | ||
| } | ||
|
|
||
| var parentClasses = context.Node.Ancestors().Where( a => a.IsKind( SyntaxKind.ClassDeclaration ) ); |
There was a problem hiding this comment.
Interesting, I hadn't considered that case. This makes sense to me (permissions applying to children.)
|
|
||
| INamedTypeSymbol baseType = context.SemanticModel.GetTypeInfo( instantiation ).Type as INamedTypeSymbol; | ||
|
|
||
| if( baseType.IsNullOrErrorType() || !baseType.Equals( classType ) ) { |
There was a problem hiding this comment.
I'd switch this to baseType == null. It can't be the error type because it got casted to INamedTypeSymbol. In generally I'm not a fan of that extension method (it's defined in this lib) because it's very imprecise.
There was a problem hiding this comment.
Ah, fair enough. Will do!
| "D2L.LP.Web.Files.FileViewing.Default.StreamFileViewerResultFactory" | ||
| }.ToImmutableHashSet(); | ||
|
|
||
| private static bool IsClassWhitelisted( string className ) { |
There was a problem hiding this comment.
It'd be slightly better to take ITypeSymbol (ISymbol?) instead of string - minorly reduces the amount of string usage (and saves the callers doing .ToString() anyway.)
|
nit: I try to keep the repo to 80 columns so diffs render good in GitHub. Don't worry about it for this PR; I'll look at setting up editorconfig today so we can just make your VS do it automatically. |
|
This looks great! I like starting simple - there's definitely room to grow this one into a more general tool for visibility control. |
|
I left some comments asking for some minor changes but looks good! |
…Symbol.IsNullOrError checks with simple null checks
|
Some of the 80 column width issues were fairly easy to correct, so I just took care of them while I was in there. |
|
|
||
| private static bool IsClassWhitelisted( ISymbol classSymbol ) { | ||
| return WhitelistedClasses.Contains( classSymbol.ToString() ); | ||
| } |
There was a problem hiding this comment.
@j3parker, I thought this was what you were getting at here, but if I've misunderstood, let me know and I'll change this up.
…rs.Visibility namespace (for ease of future generalization)
Note that the underlying interface and class referenced here (ICorsHeaderAppender and CorsHeaderAppender) aren't implemented yet. I need to wait on some dependent work to make sure the implementation actually works but, I figure it can't hurt to have the analyzer here first.
I did try to use other analyzers as a model for how to structure this stuff, but apologies in advance if there's anything I missed or didn't implement ideally. :)