-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add Microsoft Authentication helper for macOS #19
Add Microsoft Authentication helper for macOS #19
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks great. A few things to consider.
macos/Microsoft.Authentication.Helper/Source/Core/AHAppDelegate.m
Outdated
Show resolved
Hide resolved
PATH="" | ||
eval $(/usr/libexec/path_helper -s) | ||
|
||
git config --system credential.helper manager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we risk losing this configuration if Git is upgraded, like happens on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs around the Apple package installation system is pretty lacking. From my understanding I've created just a 'flat, component package' rather than a distribution package (containing multiple component packages).
Component packages have only a pre- and postinstall scripts which are run for each installation of the package. Distribution packages include more event scripts in addition to pre/postinstall (first install), such as pre/postupgrade (when a component already exists), pre/postflight (before and after the distribution package, made up of multiple component packages as a whole is installed).
If my understanding is correct (it might not be), then postinstall should be called everytime and thus GCM will be reconfigured each time.
Windows GCM is actually reconfiguring itself each time, AFAIK, because it's always invoking itself as git-credential-manager deploy
which goes through and updated all the Git installations' configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at what is required on Windows to correctly 'install' GCM, it it probably a reasonable idea to have the configuration logic sit inside GCM itself (much saner and easier to test C# than setup packages and scripts).
If that is the case, it would therefore also make sense for the configuration logic for macOS to also live inside GCM. I had opted not to do this yet however because 1) need to discuss the problems faced with the rest of the team, and 2) this current method is cheap, cheerful, and (relatively) simple/good enough for now.
Comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your assessment and understanding of the Apple package installation system matches mine, so I think this works as you expect it to.
I'm torn on if it should live inside the GCM or inside the installer, but I think the postinstall
script makes sense for now.
macos/Microsoft.Authentication.Helper/Source/Core/AHGenerateAccessToken.m
Show resolved
Hide resolved
@jamill I would appreciate your eyes on this PR too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a closer look at the obj-c later, but this is looking good.
macos/Microsoft.Authentication.Helper/Source/Core/AHAppDelegate.m
Outdated
Show resolved
Hide resolved
PKGPAYLOAD=$INSTALLEROUT/payload | ||
|
||
# Parse script arguments | ||
for i in "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This arg parser scares me. Since it's only being called by automation (other than you locally iterating on it to write it of course), can we make this simpler? Maybe just error out if $1 and $2 aren't set correctly?
PATH="" | ||
eval $(/usr/libexec/path_helper -s) | ||
|
||
git config --system credential.helper manager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your assessment and understanding of the Apple package installation system matches mine, so I think this works as you expect it to.
I'm torn on if it should live inside the GCM or inside the installer, but I think the postinstall
script makes sense for now.
Not sure if this impacts this tool. We had to revert ApexBugger to use ADAL 2.5.1 due to known issue with more recent ADAL releases that can log a non-critical message to stderr. Those messages can be misinterpreted by callers of the executables as critical errors. |
macos/Microsoft.Authentication.Helper/Source/Core/FileLogWriter.h
Outdated
Show resolved
Hide resolved
Add the initial version of the Mac Microsoft Authentication Helper using ADAL 2.5.1 via CocoaPods. The pods are statically linked so only a single executable "Microsoft.Authentication.Helper" is required to be distributed with GCM. ADAL 2.5.1 was chosen because later ADAL versions include a bug where errors were being written to standard error: AzureAD/azure-activedirectory-library-for-objc#1362
Thanks for the review @HiKumar-MS! 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now. Were you able to test with VFS for Git to make sure the new way we install fixes the issue with Git not finding credential-manager when invoked from GVFS.Mount?
Add a simple flat-package installer with postinstall script to configure the user's system gitconfig.
Update the Azure Pipelines YAML for macOS to build the Microsoft Authentication helper using Xcode and also build & publish the installer package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now after making the changes. I'm running it now and it seems to be working well!
Add the initial version of the Mac Microsoft Authentication Helper using ADAL 2.5.1 via CocoaPods. The pods are statically linked so only a single executable "Microsoft.Authentication.Helper" is required to be distributed with GCM.
Also include a script to create a macOS flat-package (.pkg) to install and configure GCM.
The Objective-C auth helper code is from the initial prototype created by @jamill.