Skip to content

Rar permissions fix#6393

Merged
Forgind merged 1 commit intodotnet:vs16.11from
Forgind:rar-permissions-fix
May 24, 2021
Merged

Rar permissions fix#6393
Forgind merged 1 commit intodotnet:vs16.11from
Forgind:rar-permissions-fix

Conversation

@Forgind
Copy link
Copy Markdown
Contributor

@Forgind Forgind commented Apr 30, 2021

Based on #6350 for convenience. That PR centralized the cache deserialization logic for StateFileBase, so it would be easy to accidentally overwrite this change with that one when resolving merge conflicts.

Context

The precomputed cache from dotnet/installer#10037 lives in Program Files after it's installed on a new computer. Program Files can only be accessed with admin privileges, which not all users have and those that have generally wouldn't expect. This permits reading the precomputed cache even without admin rights.

Changes Made

Switch how the file is read.

Copy link
Copy Markdown
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff is wonky; looks like a bunch of misrebased commits? If you just need the last commit, that looks good if you can cherry-pick to it/rebase and squash.

Note that asking for read is good hygiene even in the non-admin case.

@Forgind
Copy link
Copy Markdown
Contributor Author

Forgind commented Apr 30, 2021

Diff is wonky; looks like a bunch of misrebased commits? If you just need the last commit, that looks good if you can cherry-pick to it/rebase and squash.

I wanted to base it off of #6350 to have the logic unified before I made the change, so the extra commits are just 6350's. I can make it look pretty if you want after that's in; otherwise, squash is fine. (As you inferred, last commit was all I needed.)

@rainersigwald
Copy link
Copy Markdown
Member

I wanted to base it off of #6350 to have the logic unified before I made the change

Ah. But that's destined for 17.0 while we want this in 16.10 because it fixes the precaching feature that's new there, right?

@Forgind
Copy link
Copy Markdown
Contributor Author

Forgind commented Apr 30, 2021

I wanted to base it off of #6350 to have the logic unified before I made the change

Ah. But that's destined for 17.0 while we want this in 16.10 because it fixes the precaching feature that's new there, right?

Ugh. You're right, but that's going to make this much harder. I'll switch it to be based off of master, which should clean up the diff here, but when we merge the other one, there'll be a merge conflict, and we need to make sure to not do the obvious thing of "method was deleted --> delete change, too" and instead integrate the change into StateFileBase as I did here.

new FileStream(stateFile, FileMode.Open) opens the file as if you had read/write access but only actually grants you read permissions. It still requires administrator privileges, however, if a file requires administrator privileges to write to. This removes that requirement.
@Forgind Forgind force-pushed the rar-permissions-fix branch from 905a7e5 to 2012309 Compare May 2, 2021 16:00
@Forgind
Copy link
Copy Markdown
Contributor Author

Forgind commented May 2, 2021

I updated it to not be based on #6350 based on @rainersigwald's comment above.

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label May 6, 2021
@rainersigwald
Copy link
Copy Markdown
Member

Did we decide to put this in 16.11, or 17.0? I have already forgotten 😬

@Forgind
Copy link
Copy Markdown
Contributor Author

Forgind commented May 11, 2021

Did we decide to put this in 16.11, or 17.0? I have already forgotten 😬

16.11. We wanted to be able to specify that VS can use the latest 16.11 version rather than requiring 17.0 after the feature is turned on.

@Forgind Forgind changed the base branch from main to vs16.11 May 11, 2021 16:53
@Forgind Forgind merged commit 1c6e7ad into dotnet:vs16.11 May 24, 2021
@Forgind Forgind deleted the rar-permissions-fix branch May 24, 2021 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants