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

Kagre preserve working path on impersonate #3

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Kagre
Copy link

@Kagre Kagre commented Dec 29, 2020

Take a look, this is a breaking change, but hopefully for the better.

The idea here was to remain in the current on the same working path as long as the new context still had access to it.

Also, if you don't add it in; please at least fix the assumption that when you pop the context back from admin the original user has access to wherever admin wandered off to...

Kagre added 15 commits December 29, 2020 13:42
preserve the current working path if possible, otherwise fall back to the root on the system drive
Force the current path to be the root on the system drive before switching contexts
restore working path prior to Push-ImpersonationContext
forgot this one on the last commit
name the path stack so that it doesn't interfere with the user's pushd stack
name the path stack so that it doesn't interfere with the user's pushd stack
functional --> fanciful: match the project's coding style
functional --> fanciful, update to match the project's coding style
.INPUTS is for the pipe-stream, not the parameters
somehow in my edits, had gotten the order of these reversed
apparently not all non-interactive contexts are created equal...

The context in which the appveyor auto-build is running should be non-interactive; however the old code failed to detect this; so as patch I've added a way for the caller to explicitly state that Use-VaultSecureString should have been invoked as non-interactive.
the build fails, this is hopefully a workaround; however the build should probably not use the project it's building
appveyor build updates
@Kagre
Copy link
Author

Kagre commented Dec 30, 2020

This branch may now try to fix too much. I was hoping to eliminate the build failure, but now the issue is probably that the version imported is older than the one being built... still it's not a compiler probably shouldn't be using it to build itself.

@claudiospizzi
Copy link
Owner

@Kagre
Thank you for your PR - absolutely valid to have a consistent location. Can you check my comments?

@Kagre
Copy link
Author

Kagre commented Feb 8, 2021

Sorry, I'm still not super familiar with git/hub... where might I find these comments?

@@ -3,5 +3,5 @@
. InvokeBuildHelperTasks

# Build configuration
$IBHConfig.RepositoryTask.Token = Use-VaultSecureString -TargetName 'GitHub Token (claudiospizzi)'
Copy link
Owner

Choose a reason for hiding this comment

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

Yes this is breaking the build script as the module requires do be released before it's actually used.

Copy link
Author

Choose a reason for hiding this comment

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

Do I need to figure out how to back these two out of the pr?

# Used to explicitly set NonInteractive mode
[Parameter(Mandatory = $false)]
[switch]
$NonInteractive
Copy link
Owner

Choose a reason for hiding this comment

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

Idea is good: I will implement this in a dedicated change also including the Use-VaultCredential which has the same behavior. Also I consider throwing a warning if we were not able to retrieve the credentials in non interactive mode. Should be find for the build system.

@claudiospizzi
Copy link
Owner

@Kagre My bad, didn't submit the review. Should now be visible in this pr.

Convert the try/catch/finally to use Test-Path instead
on second thought let the error pass through depending on the $ErrorActionPreference

if the user chooses to ignore it then they can do so on their end
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.

2 participants