Skip to content

Conversation

peombwa
Copy link
Member

@peombwa peombwa commented Oct 5, 2022

This PR closes #1366 by adding -Credential parameter of type PSCredential to Connect-MgGraph command.

The parameter is being added to:

Proposes Usage

image

@peombwa peombwa self-assigned this Oct 5, 2022
@peombwa peombwa marked this pull request as ready for review October 5, 2022 23:01
@peombwa peombwa merged commit facfbf7 into features/2.0 Oct 12, 2022
@peombwa peombwa deleted the features/SecretsClientCredentials branch October 12, 2022 21:13
authContext.TokenCredentialType = TokenCredentialType.UserProvidedAccessToken;
authContext.ContextScope = ContextScope.Process;
GraphSession.Instance.InMemoryTokenCache = new InMemoryTokenCache(Encoding.UTF8.GetBytes(new NetworkCredential(string.Empty, AccessToken).Password));
authContext.AuthType = AuthenticationType.AppOnly;
Copy link

@nkasco nkasco Nov 26, 2022

Choose a reason for hiding this comment

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

Won't this cause user confusion when users inevitably try to use -Credential within Connect-MgGraph for ROPC / User Credential auth flow use with delegated permission types? I get that you're only intending to add support for client secrets / App Only, I just think in the process it might be creating UX confusion.

Perhaps an alias or rename of the parameter could aleviate this. -ClientSecret, -ClientSecretCredential, -AppCredential, or -AppSecret might be a few possibilities.

#125 (comment) - Looks like it already has created some confusion unless I've completely misunderstood the code. Keep me honest here.

@grzegorzwierzbicki
Copy link

grzegorzwierzbicki commented Feb 3, 2023

This is completely wrong and confusing
-Credential should accept username and password for accessing the application with delegated permissions.

#1366 and similar should not be closed as the expectation was to add support for non-interactive username/password login

@grzegorzwierzbicki
Copy link

On top of that, the v2 module no longer accepts tokens acquired with MSAL.PS
Why are you doing this ?
Why are you working so hard on making life miserable for anyone who just wants to use Graph in a script.

$AccessToken = Get-MsalToken -ClientId $PowerShellGraphAppID -TenantId $TenantId -UserCredential $creds -Scopes "User.ReadWrite.All"
Connect-MgGraph -AccessToken $AccessToken.AccessToken
Connect-MgGraph : Cannot bind parameter 'AccessToken'. Cannot convert the "eyJ....5bA" value of type "System.String" to type "System.Security.SecureString"

@nkasco
Copy link

nkasco commented Feb 3, 2023

On top of that, the v2 module no longer accepts tokens acquired with MSAL.PS

Why are you doing this ?

Why are you working so hard on making life miserable for anyone who just wants to use Graph in a script.


$AccessToken = Get-MsalToken -ClientId $PowerShellGraphAppID -TenantId $TenantId -UserCredential $creds -Scopes "User.ReadWrite.All"

Connect-MgGraph -AccessToken $AccessToken.AccessToken

Connect-MgGraph : Cannot bind parameter 'AccessToken'. Cannot convert the "eyJ....5bA" value of type "System.String" to type "System.Security.SecureString"

You need to convert the string token to secure string to use with the v2 module. That was documented in the breaking changes. MSAL.PS isn't broken.

That said, I do agree about the credentials parameter being confusing as I documented above.

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.

Add -Credential as authentification method to the Connect-MgGraph Command

4 participants