dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.87k stars 779 forks source link

Remove type provider warning dialog #378

Closed enricosada closed 9 years ago

enricosada commented 9 years ago

When i add a type provider assembly i get this scary warning

image

That's ok, is a security issue. The warning is by assembly location, i get this every project X type provider

But there are other components inside visual studio with the same issue.

When i add a Text Template (t4) i get this warning dialog. There is a checkbox do not show this message again ( i found the image on google, i suppressed the warning long time ago ). Only one time, per user

image

No warning with:

I'd like to remove the warning dialog, but maybe we can just disable it:

i can implement the code.

I really hate this dialog :smile: because when i explain the f# strenghts the type providers seem dangerous

ovatsus commented 9 years ago

This dialog box also really annoys me. Some times you clone a project and build, and then there is a nuget restore and everything fails because this dialog will only show up after the build ends, because when you opened the project the TP reference wasn't there yet

Having a don't ask again checkbox would be great

forki commented 9 years ago

I'm not sure who makes such decisions at Microsoft, but I assume if we want to change that then the T4 way is our best bet. That said: the security concerns are valid, but then every nuget package had to come with such a disclaimer. Especially since NuGet is running ps1 files during install.

philderbeast commented 9 years ago

That I can build solutions on the command line without a type provider warning or error but from the IDE I get a warning dialog and a build failure is not consistent.

forki commented 9 years ago

Sssh please don't tell anyone about this. They might change this and break our CI builds ;-)

vasily-kirichenko commented 9 years ago

:+1: for the checkbox. The dialog is very annoying.

dsyme commented 9 years ago

Yes, I feel a checkbox seems like a reasonable way to make progress.

enricosada commented 9 years ago

remove it is the title :fire: :fire: , but i can settle for checkbox :smile:

allykzam commented 9 years ago

Jumping in from F# weekly, the most annoying part of this dialog for me is that if I use the same type provider in two different projects, I get the dialog once for each project because the binary has a different file path. I assume the OSS type providers aren't distributed as signed binaries, so perhaps Visual Studio could track type providers by their assembly name and checksums of the binary files that come with it?

I'd love to be able to just turn off the dialog box as suggested, but if a safer solution is needed, that's my 2¢

rojepp commented 9 years ago

@amazingant Then you'd have this dialog for each nuget/paket update instead.

Authorisation per Nuget package id would be ideal (If I trust V1 I'm likely to trust V2), but you don't always have one.

The suggested checkbox solution seems fine. I'm going to run the code anyway. If it is a TP I'll run the code directly in the IDE. If it is not, I am still going to run the code at run-time. And, as @enricosada said, in the case of a PS script, I'm running arbitrary code on my machine anyway just by installing the package. There is some inherent trust when installing a package.

allykzam commented 9 years ago

@rojepp You say that, but I've yet to update a type provider. I've installed the same ones via NuGet to a dozen different projects and encountered this dialog over and over again due to it validating against the path. Although again, if I can just turn it off altogether as previously suggested, I'm happy with that too. :)

rojepp commented 9 years ago

I update all the time, especially when contributing fixes. :) We all realize we're not getting rid of it, so checkbox seems a fine solution!

KevinRansom commented 9 years ago

We would certainly consider incorporating a check box to suppress further display of this dialog on a per user basis. I would regard the risk of regressions as low, and so I believe we would take the fix for VS 2015 if a PR arrives over the next week or two. The later it arrives, the more likely it gets pushed to V.Next. At this point the PR should contain the minimum necessary change to add the check box. Please note that getting the necessary internal approval for the change this late means that it is not guaranteed to make it into VS 2015 but we can try. The label will need localization, that's is our responsibility not yours, but it greatly impacts our ability to take the fix.

enricosada commented 9 years ago

Great @KevinRansom , let's try for vs2015. I know this is an exception because is not a bug but an improvement, really thx for the effort.

As i said i can do it for sure next week (monday or tuesday), but if someone want to step in as first commit that's a great up-for-grab.

enricosada commented 9 years ago

no volunteers so i'll do it :smile:

latkin commented 9 years ago

If you are modifying the dialog, make sure to check various test cases:

latkin commented 9 years ago

FYI we are discussing options here with the VS security signoff people (we can't just reduce/remove a security dialog without review). We will update when there is something worth sharing.

enricosada commented 9 years ago

btw @latkin can you ask (instead of checkbox) if we can user the current behavior of nuget or roslyn analizers?

nuget

no warning

example of execute script on install

Executing script file 'D:\temp\temp projects\ConsoleApplication1ref2015\ConsoleApplication3\packages\ClrHeapAllocationAnalyzer.1.0.0.6\tools\install.ps1'...

roslyn analyzer

add nuget -> enabled by default and work without warning

the green is the analizer image

image

no security, only enable, disable, by class name image

latkin commented 9 years ago

image

We have the green light to completely remove the security dialog.

We are still asked to provide some kind of indication that referenced type providers are "special," which IMO is reasonable. I suggested that we use a special icon in the solution explorer, similar to how Roslyn analyzers are displayed, and that was judge to be fine.

Anyone happen to know if it's simple to use a custom icon for certain references in solution explorer?

rojepp commented 9 years ago

Great!

But still, displaying a special indication because of security concerns seems a bit silly. By the time you see it, it is too late, the untrusted TP will already be executing due to intellisense etc. Your gfx driver may already be hacked so that you are unable to see the warning. Indeed, you may no longer be able to operate your OS at all by the time you are supposed to be seeing that indication! :)

latkin commented 9 years ago

@rojepp It's primarily about usability, not security

allykzam commented 9 years ago

@rojepp and this is why you don't run Visual Studio as Administrator. ;)

rojepp commented 9 years ago

You mean the visual indication? The current enable/disable prompt is for security, not usability? :) Note, I am glad about this! Installing a TP or even a NuGet package at all is trust. Both can f*ck up your machine or data without even compiling code. @amazingant I do run VS as Administrator, because of work complexities. Administrator or not, you still stand to loose valuable data if you install the wrong NuGet package. IMHO, Data is much more valuable than the installation. You and I know to not store anything important on the machine but many don't.

@latkin What is the usability of indicating that the reference is a TP?

allykzam commented 9 years ago

@rojepp I do too, due to half our application's settings still being in the registry. :cry:

You and I know not to store anything important on the machine

You're saying this to someone who lets VS remember his credentials to the production server. :laughing:

ovatsus commented 9 years ago

I think it would be great if references with TPs have a different icon on the solution explorer

vasily-kirichenko commented 9 years ago

@ovatsus I agree. And assemblies from nuget packager would be given another icon, too. Or two sunflowers could be added - "Type Providers" and "NuGet" (the latter is like in xamarin).

enricosada commented 9 years ago

Great @latkin !!

image

I like the idea of a different icons, type providers are assemblies with super powers.

I'll close #427 and send other two pw

we can also add a new entry 'TypeProvider: True/False' in assembly property panel

image

allykzam commented 9 years ago

If assemblies from NuGet packages are going to get an icon as @vasily-kirichenko suggested, I'd say something like :package: but with the top opened would be nice; I don't have any bright ideas for the type provider icon other than maybe it should use the blue F# logo for the light and blue VS themes, and one of the light logos for the dark theme?

Blue logo: blue F# logo

And the light one (right-click and open somewhere else, near-impossible to see on a white background): light F# logo

enricosada commented 9 years ago

@vasily-kirichenko i dont know how to discriminate local assembly vs nuget assembly. check assembly location parent directories for .nupkg? that another issue, but is a really nice idea

tp is easier, because all i need is the .dll

btw, about the icon, i added #448 so we can iterate feedback on the icon

@latkin remove security dialog mean remove also the trusted tp list?

image

latkin commented 9 years ago

@enricosada yeah, I suppose the whole approval menu gets the axe, too.

I imagine lots of roots to dig out on this one.

dsyme commented 9 years ago

I'll be thrilled to see the tpApprovals code path through to est.fs axed as well, it was a horrible hack into the compiler

enricosada commented 9 years ago

The est.fs code is like to stay ( We discuss tomorrow with my pr ) because i dont know how to diccriminate if amo assenbly is a type provider or not. I tried Assembly.ReflectionOnlyLoad but not work with custom attributes ( need to load deps ) https://msdn.microsoft.com/en-us/library/ms172331%28v=vs.110%29.aspx

I'll try compiler service directly

latkin commented 9 years ago

@enricosada I would assume you can just take whatever existing code performs "this ref has a TP, show the dialog" and re-use it to perform "this ref has a TP, change the icon". Hopefully no need to add new logic. Maybe that's what you are saying? At least for short term hopefully we can re-used code.

enricosada commented 9 years ago

exactly @latkin i'll use the old logic.

my plans were:

a) check if exists assembly TypeProviderAttribute

this doesnt work right (Assembly.ReflectionOnlyLoad doesn't help) an anyway that was a wrong idea, because is not responsibility of project system to understand type providers logic.

b) reuse old logic

same as before, reusing tpApprovals code path, the ugly hack remain ( implementing this now ), instead of choose dialog, i set the AssemblyReferenceNode.IsTypeProvider property

c) ask compilerservice after build

after successful build, i ask compiler service if an assembly reference is type provider or not.

pro: this remove the old hack in est.fs (vNext or this pr but the priority is removing the dialog in vs2015 , if possible )

latkin commented 9 years ago

:+1:

latkin commented 9 years ago

The dialog has been removed! Thanks @enricosada.

Per my comments in #448, for now I just pulled everything out since there did not seem to be an easy way to reliably flag a reference as a TP at design time. We can definitely go back and implement this as a separate step though.

dsyme commented 9 years ago

For posterity, here's how the first version of the dialog looked ,circa 2011.... A long journey to nothingness :)

old-dialog

vasily-kirichenko commented 9 years ago

@dsyme wow :) @latkin thanks a lot! Pressing the OK button 15 times each time our solution is build on a clean repo is an awful experience.

enricosada commented 9 years ago

@dsyme i get less intrusive warnings downloading and executing a fake flash update.. :smiley: