Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove type provider warning dialog #378

Closed
enricosada opened this issue Apr 22, 2015 · 38 comments
Closed

Remove type provider warning dialog #378

enricosada opened this issue Apr 22, 2015 · 38 comments

Comments

@enricosada
Copy link
Contributor

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:

  • nuget, who can execute custom powershell script on install
  • roslyn analizer
  • test runner from nuget packages (like xunit.runner.visualstudio )
  • package manager inside visual studio like npm, python etc

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

  • ok: add checkbox like t4
  • best: disable this dialog by default, enable it with in visual studio f# config
  • complicated: rules by assemblyname/publisher/nuget

i can implement the code.

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

@ovatsus
Copy link

ovatsus commented Apr 22, 2015

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
Copy link
Contributor

forki commented Apr 23, 2015

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
Copy link

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
Copy link
Contributor

forki commented Apr 23, 2015

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

@vasily-kirichenko
Copy link
Contributor

👍 for the checkbox. The dialog is very annoying.

@dsyme
Copy link
Contributor

dsyme commented Apr 23, 2015

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

@enricosada
Copy link
Contributor Author

remove it is the title 🔥 🔥 , but i can settle for checkbox 😄

@allykzam
Copy link

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
Copy link
Contributor

rojepp commented Apr 28, 2015

@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
Copy link

allykzam commented Apr 28, 2015 via email

@rojepp
Copy link
Contributor

rojepp commented Apr 28, 2015

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

no volunteers so i'll do it 😄

@latkin
Copy link
Contributor

latkin commented May 4, 2015

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

  • 200% high DPI
  • Check box can be selected/toggled via keyboard
  • Windows narrator can read the label text

@latkin
Copy link
Contributor

latkin commented May 8, 2015

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
Copy link
Contributor Author

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
Copy link
Contributor

latkin commented May 12, 2015

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
Copy link
Contributor

rojepp commented May 12, 2015

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
Copy link
Contributor

latkin commented May 12, 2015

@rojepp It's primarily about usability, not security

@allykzam
Copy link

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

@rojepp
Copy link
Contributor

rojepp commented May 12, 2015

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
Copy link

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

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. 😆

@ovatsus
Copy link

ovatsus commented May 12, 2015

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

@vasily-kirichenko
Copy link
Contributor

@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
Copy link
Contributor Author

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

  • code removal
  • icon, so we can iterate on icon image 😄

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

image

@allykzam
Copy link

If assemblies from NuGet packages are going to get an icon as @vasily-kirichenko suggested, I'd say something like 📦 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
Copy link
Contributor Author

@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
Copy link
Contributor

latkin commented May 13, 2015

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

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

@dsyme
Copy link
Contributor

dsyme commented May 13, 2015

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
Copy link
Contributor Author

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
Copy link
Contributor

latkin commented May 13, 2015

@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
Copy link
Contributor Author

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
Copy link
Contributor

latkin commented May 14, 2015

👍

@latkin latkin added this to the F# 4.0 Update 1 milestone Aug 4, 2015
@latkin latkin added the fixed label Aug 19, 2015
@latkin
Copy link
Contributor

latkin commented Aug 19, 2015

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.

@latkin latkin closed this as completed Aug 19, 2015
@dsyme
Copy link
Contributor

dsyme commented Aug 20, 2015

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

old-dialog

@vasily-kirichenko
Copy link
Contributor

@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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants