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

Write permission for Users to global persist dir #2524

Merged
merged 8 commits into from
Aug 26, 2018
Merged

Write permission for Users to global persist dir #2524

merged 8 commits into from
Aug 26, 2018

Conversation

Retia-Adolf
Copy link
Contributor

@rasa
Copy link
Member

rasa commented Aug 23, 2018

files have lines containing trailing whitespace.
RuntimeException: The following 1 lines contain trailing whitespace:
File: C:\projects\scoop\lib\install.ps1, Line: 58

@r15ch13
Copy link
Member

r15ch13 commented Aug 26, 2018

This will fail on a German PC 😁

Exception calling "SetAccessRule" with "1" argument(s): "Some or all identity references could not be translated."
At line:1 char:1
+ $acl.SetAccessRule((New-Object System.Security.AccessControl.FileSyst ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : IdentityNotMappedException

Output of Get-Acl without the correct permission:

λ (Get-Acl 'C:\ProgramData\scoop\persist').Access

FileSystemRights  : FullControl
AccessControlType : Allow
IdentityReference : NT-AUTORITÄT\SYSTEM
IsInherited       : True
InheritanceFlags  : ContainerInherit, ObjectInherit
PropagationFlags  : None

FileSystemRights  : FullControl
AccessControlType : Allow
IdentityReference : VORDEFINIERT\Administratoren
IsInherited       : True
InheritanceFlags  : ContainerInherit, ObjectInherit
PropagationFlags  : None

FileSystemRights  : 268435456
AccessControlType : Allow
IdentityReference : ERSTELLER-BESITZER
IsInherited       : True
InheritanceFlags  : ContainerInherit, ObjectInherit
PropagationFlags  : InheritOnly

FileSystemRights  : ReadAndExecute, Synchronize
AccessControlType : Allow
IdentityReference : VORDEFINIERT\Benutzer
IsInherited       : True
InheritanceFlags  : ContainerInherit, ObjectInherit
PropagationFlags  : None

FileSystemRights  : Write
AccessControlType : Allow
IdentityReference : VORDEFINIERT\Benutzer
IsInherited       : True
InheritanceFlags  : ContainerInherit
PropagationFlags  : None

It only works by setting the user to VORDEFINIERT\Benutzer, which adds:

FileSystemRights  : Write, Synchronize
AccessControlType : Allow
IdentityReference : VORDEFINIERT\Benutzer
IsInherited       : False
InheritanceFlags  : ObjectInherit
PropagationFlags  : None

@Retia-Adolf
Copy link
Contributor Author

??! I had thought these user/group names are all the same. Never see differences on a Chinese PC etc.

I think I need to find a way to get the actual Users group name, else it will continue to fail on other European PCs...

@r15ch13
Copy link
Member

r15ch13 commented Aug 26, 2018

Found a solution https://stackoverflow.com/questions/40587096/set-output-language-of-get-acl#40588213
$user = New-Object System.Security.Principal.SecurityIdentifier 'S-1-5-32-545'

@r15ch13
Copy link
Member

r15ch13 commented Aug 26, 2018

Fixed it Not yet!

Call:

    # persist data
    persist_data $manifest $original_dir $persist_dir
    persist_permission $manifest $global

Function:

# check whether write permission for Users usergroup is set to global persist dir, if not then set
function persist_permission($manifest, $global) {
    if ($manifest.persist -and !$global) {
        return
    }
    $path = persistdir $null $global
    $user = New-Object System.Security.Principal.SecurityIdentifier 'S-1-5-32-545'
    $target_rule = New-Object System.Security.AccessControl.FileSystemAccessRule($user, 'Write', 'ObjectInherit', 'none', 'Allow')
    $acl = Get-Acl -Path $path
    $acl.SetAccessRule($target_rule)
    $acl | Set-Acl -Path $path
}

Edit: Fixed the persistence and global check

@r15ch13
Copy link
Member

r15ch13 commented Aug 26, 2018

Fixed the code (See above)
Only providing the SecurityIdentifier works fine (see stackoverflow)
Checking if the permission is already set doesn't work. Adding it again does no harm.

Copy link
Member

@r15ch13 r15ch13 left a comment

Choose a reason for hiding this comment

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

...

lib/install.ps1 Outdated
if ($persist -and $global) {
$path = "$(basedir $global)\persist"
$user = -join ([System.Environment]::MachineName, '\',
(([System.Security.Principal.SecurityIdentifier]'S-1-5-32-545').Translate([System.Security.Principal.NTAccount])).Value)
Copy link
Member

Choose a reason for hiding this comment

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

This would result in: R15CH13-PC\VORDEFINIERT\Benutzer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, I wrongly took domain name as computer name before.
(((Sorry, I don't know coding, and actually copy codes in function persist_permission from here.

Copy link
Member

Choose a reason for hiding this comment

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

The important part is "testing"
I just installed and uninstalled notepad++ with these changes about 30 times now 😁

@r15ch13 r15ch13 merged commit fc1c2a6 into ScoopInstaller:master Aug 26, 2018
@Retia-Adolf
Copy link
Contributor Author

Retia-Adolf commented Aug 26, 2018

@r15ch13 Checking permission seems no problem for me:

$path = $PWD$path

Path
----
C:\Users\Retia\Git\Retia-Adolf

❯ $user = [System.Security.Principal.SecurityIdentifier]'S-1-5-32-545'$Rights = "Write"$InheritSettings = "ObjectInherit"$PropogationSettings = "none"$RuleType = "Allow"$acl = Get-Acl -Path $PWD$perm = $user, $Rights, $InheritSettings, $PropogationSettings, $RuleType$targetRule = New-Object -TypeName System.Security.AccessControl.FileSystemAccessRule -ArgumentList $perm$isSet = $trueForEach ($existRule in $acl) {
>>     $isSet = ($existRule -match $targetRule) -and $isSet
>> }
❯ $isSet
False

So !($isSet) will equal $true, then if (!($isSet)) should perform. Maybe we could add these codes back:

        $isSet = $true
        ForEach ($existRule in $acl) {
            $isSet = ($existRule -match $targetRule) -and $isSet
        }
        if (!$isSet) {
            $acl.SetAccessRule($targetRule)
            $acl | Set-Acl -Path $path
        }

@r15ch13
Copy link
Member

r15ch13 commented Aug 26, 2018

This can't work, because it should be ForEach ($existRule in $acl.Access) instead of ForEach ($existRule in $acl).
Comparing the rules with -match also doesn't work. Add debug ($existRule -match $targetRule) to the Loop and you will see that it always outputs True which it shouldn't.

    ForEach ($existRule in $acl.Accesss) {
        debug ($existRule -match $target_rule)
        $isSet = ($existRule -match $target_rule) -and $isSet
    }
    if (!$isSet) {
        debug "SetAccessRule"
        $acl.SetAccessRule($target_rule)
        $acl | Set-Acl -Path $path
    }

@Retia-Adolf
Copy link
Contributor Author

Thanks for explaining

@Ash258
Copy link
Contributor

Ash258 commented Aug 27, 2018

Downloads $ scoop install sysinternals
Installing 'sysinternals' (July.5.2018) [64bit]
SysinternalsSuite.zip (24.0 MB) [=============================================================================] 100%
Checking hash of SysinternalsSuite.zip ... ok.
Extracting SysinternalsSuite.zip ... done.
Linking S:\Scoop\apps\sysinternals\current => S:\Scoop\apps\sysinternals\July.5.2018
Creating shim for 'accesschk'.
...Lots of shims...
Creating shortcut for SysInternals/ProcessExplorer - Enhanced Task Manager (procexp64.exe)
...Lots of shortcuts...
Set-Acl : Attempted to perform an unauthorized operation.
At S:\Scoop\apps\scoop\current\lib\install.ps1:1235 char:12
+     $acl | Set-Acl -Path $path
+            ~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : PermissionDenied: (S:\Scoop\persist\:String) [Set-Acl], UnauthorizedAccessException
    + FullyQualifiedErrorId : System.UnauthorizedAccessException,Microsoft.PowerShell.Commands.SetAclCommand

@r15ch13
Copy link
Member

r15ch13 commented Aug 27, 2018

@Ash258 oh shit! Fixed with eb7b7cb

@Ash258
Copy link
Contributor

Ash258 commented Aug 27, 2018

@r15ch13 Everything works fine now 😍

@Retia-Adolf
Copy link
Contributor Author

Retia-Adolf commented Aug 28, 2018

Oops, I didn't consider the case that standard user installs app into a custom global app's directory. But just wondering if you don't have rights to modify permission, isn't it impossible to add system environment variable?

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 this pull request may close these issues.

None yet

4 participants