pnp / powershell

PnP PowerShell
https://pnp.github.io/powershell
MIT License
679 stars 348 forks source link

[BUG] Connect-PnPOnline with App Id and CertificateBase64Encoded failing with PnP.PowerShell 1.11.0 #2071

Closed pmatthews05 closed 2 years ago

pmatthews05 commented 2 years ago

Reporting an Issue or Missing Feature

When connecting to SharePoint using an Azure AppId and Certificate using the Certificatebase64Encoded, it fails to obtain connection. This was working without issue in PnP.PowerShell 1.10.0

Expected behavior

When connecting with Azure AppId and Certificate using the Certificatebase64Encoded it connects, and obtain context.

Actual behavior

When using a clientId and Certificatebase64Encoded value.

PS C:\Code> $tenantName = "<tenantName>"
PS C:\Code> $clientId = "<clientId>"
PS C:\Code> $cert64String = "<MIIKUAIBAzCCCgwGCSqGSIb3DQEHAaCC.........>"
PS C:\Code> $sharePointAdminConnection = Connect-PnPOnline `
 -Url:"https://$tenantName-admin.sharepoint.com" `
 -Tenant:"$tenantName.onmicrosoft.com" `
 -ClientId: $clientId `
 -CertificateBase64Encoded:$cert64String `
 -ReturnConnection
VERBOSE: PnP PowerShell Cmdlets (1.11.0)
PS C:\Code> Get-PnPConnection
Get-PnPConnection: The current connection holds no SharePoint context. Please use one of the Connect-PnPOnline commands which uses the -Url argument to connect.
PS C:\Code> Get-PnpWeb
Get-PnPWeb: The current connection holds no SharePoint context. Please use one of the Connect-PnPOnline commands which uses the -Url argument to connect.
PS C:\Code> Get-PnpContext
Get-PnPContext: The current connection holds no SharePoint context. Please use one of the Connect-PnPOnline commands which uses the -Url argument to connect.

Please note Using PnPManagementShell works with no issue.

$tenantName = "<PutTenantName>"
Connect-PnpOnline -Url:https://$tenantName-admin.sharepoint.com -PnPManagementShell
WARNING: 

To sign in, use a web browser to open the page https://microsoft.com/devicelogin and enter the code XXXXXXXXX to authenticate.

VERBOSE: PnP PowerShell Cmdlets (1.11.0)
PS C:\Code> Get-PnpConnection

ConnectionType                         : O365
InitializationType                     : Unknown
Scopes                                 : 
PSCredential                           : 
ClientId                               : 31359c7f-bd7e-475c-86db-fdb8c937548e
ClientSecret                           : 
ApplicationInsights                    : PnP.PowerShell.ALC.ApplicationInsights
Url                                    : https://$tenant-admin.sharepoint.com/
TenantAdminUrl                         : 
Certificate                            : 
DeleteCertificateFromCacheOnDisconnect : False
Context                                : PnP.Framework.PnPClientContext
Tenant                                 : 
AzureEnvironment                       : Production

PS C:\Code> Get-PnpWeb

Title                 ServerRelativeUrl Id
-----                 ----------------- --
Tenant Administration /                 xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx

PS C:\Code> Get-PnpContext

RetryCount                : 10
Delay                     : 500
PropertyBag               : {}
Web                       : Microsoft.SharePoint.Client.Web
Site                      : Microsoft.SharePoint.Client.Site
RequestResources          : Microsoft.SharePoint.Client.RequestResources
ServerVersion             : 16.0.22615.12004
Url                       : https://$tenant-admin.sharepoint.com/
ApplicationName           : .NET Library
ClientTag                 : PnPPS:1.11:Get-PnPWeb
DisableReturnValueCache   : True
ValidateOnClient          : True
Credentials               : 
WebRequestExecutorFactory : Microsoft.SharePoint.Client.DefaultWebRequestExecutorFactory
PendingRequest            : Microsoft.SharePoint.Client.ClientRequest
HasPendingRequest         : True
Tag                       : 
RequestTimeout            : 180000
StaticObjects             : {[SharePointPnP$Settings$ContextCloning, PnP.Framework.Utilities.Context.ClientContextSettings], [Microsoft$SharePoint$SPContext$Current,
                            Microsoft.SharePoint.Client.RequestContext]}
ServerSchemaVersion       : 15.0.0.0
ServerLibraryVersion      : 16.0.22615.12004
RequestSchemaVersion      : 15.0.0.0
TraceCorrelationId        : 3aca4ca0-d007-4000-4cfe-466b9ed3acb3

Steps to reproduce behavior

Please see above

What is the version of the Cmdlet module you are running?

ModuleType Version    PreRelease Name                                PSEdition ExportedCommands
---------- -------    ---------- ----                                --------- ----------------
Manifest   1.11.0                PnP.PowerShell                      Core,Desk {Add-PnPAdaptiveScopeProperty, Add-PnPPropertyBagValue, Add-PnPSiteClassification, Copy-PnPFolder…}

Which operating system/environment are you running PnP PowerShell on?

steven-hyland commented 2 years ago

I am experiencing a similar problem with the v1.11.0 library failing to acquire a connection. In my case, my script is signing in with a user/password credential:

$spPw = ConvertTo-SecureString $env:SP_PW -Asplaintext -Force
$spCredential = New-Object System.Management.Automation.PSCredential ($env:SP_USER,$spPw)
$spConnection = Connect-PnPOnline -Url $SiteUrl -Credentials $Credential -ReturnConnection -WarningAction Ignore

Using $spConnection in any other cmdlets (such as Set-PnpListItem) returns an error similar to the one in the OP: The current connection holds no SharePoint context. Please use one of the Connect-PnPOnline commands which uses the -Url argument to connect. This occurs on both my local machine (win 10) and our agent machines (win server 2016).

I have adjusted our pipeline to only use PnP.PowerShell v1.10.0 and it's working fine now. I guess this is on us for not locking to a particular version, but at the same time I don't expect breaking changes in a minor release version bump.

KoenZomers commented 2 years ago

This has indeed changed in the latest release. There was a bug in the Connect-PnPOnline code that would change the current context even when you were using -ReturnConnection, which it should not be doing. For your scenario, simply remove the -ReturnConnection from your Connect-PnPOnline and you should be good.

The idea here is that you only return the connection if you want to assign it to a variable to keep it as a second, third, fourth, etc... connection to use in your script. In this scenario you would need to provide -Connection $var with every cmdlet you execute to instruct it to use explicitly that connection. This could be useful if you work with different permissions or authentication types. If you don't intend to do any of this and just want one connection, do not provide -ReturnConnection. It will then set the connection on the default connection and none of your PnP PowerShell cmdlets will need to have -Connection $var provided.

pmatthews05 commented 2 years ago

@KoenZomers I require the ReturnConnection to use later.

KoenZomers commented 2 years ago

In that case you will have to provide -Connection with every cmdlet. You cannot and should not mix the current context with a specific context. Its either or.

pmatthews05 commented 2 years ago

I thought the purpose of it was to hold it in a variable, then use Set-PnPContext to switch, or pass it in -Connection. What you are saying, is as soon as you use the -ReturnConnection every command after that needs to have -Connection until you disconnect and reconnect without -ReturnConnection?

KoenZomers commented 2 years ago

That's correct. I realize it is a big change to how things used to work, but if you think about it, it makes way more sense this way. You can now really nicely keep things separated and work with as many different connections, authentication types and permissions as you wish. You simply pass in the proper connection variable. That's why we have expanded the -Connection parameter to nearly all cmdlets now, to support this. If you don't want any of this, which will account for 90% of the scenarios, you simply connect and work with the default context and will not have to pass in any connection anywhere.

sebastianrogers commented 2 years ago

Trying to understand this breaking change.

Old Behaviour

Old Behaviour

Would it not be better to retain the previous behaviour for backwards compatibility and introduce either a new set of parameters for the new behaviour or a new set of commands?

As it is this change breaks nearly all of our scripts.

steven-hyland commented 2 years ago

@KoenZomers thank you for the additional context and information, that makes more sense now. I reviewed our scripts and confirmed that some call sites were not using the stored $spConnection variable, which is where we saw the errors. I still expect to see breaking behavior changes in a major version bump (such as v2.0.0), keeping with semver standards.

KoenZomers commented 2 years ago

I understand your concerns, Steven. It was one in a grey area for us. We try to support backwards compatibility as much as possible but your scenario is actually a perfect example why we decided to classify this as a bug and fix it already. As you mention you found out that you didn't supply a specific connection with all of the cmdlets. So they were relying on the last connection you made, regardless if it was using ReturnConnection or not. Especially in larger scripts, especially after a while, especially if someone is tasked with modifying the script that didn't initially make it, this can very easily lead to errors. Think about adding a seemingly innocent section in the middle of the script connecting to some central site collection and updating a list item with a status. It could or would impact all the cmdlets later on in the script in the old situation. Now it has become a much more conscious action on which connection you execute something.

It really should never have been released like how it used to work. Version 2 was too far away and would entail too many other changes which made us decide not to be able to wait for it with this.

steven-hyland commented 2 years ago

To clarify my position - I agree with you that the previous behavior was buggy and error-prone, was hard to use correctly, and probably should not have been released that way. I also appreciate the nuances of the judgement call involved here. So just for the record, my primary objection is more around user trust.

In my experience, it's common to write code to work around the lack of version specifiers available in PowerShell's *-Module cmdlets (compared to Node's package.json, for example). However, I can no longer trust code like the following (simplified example) to get and use the best available non-breaking version of this library:

$version = (Find-Module PnP.PowerShell -Repository PSGallery -AllVersions | Where-Object { $_.Version.StartsWith("1.") } | Select-Object -First 1).Version
Install-Module PnP.PowerShell -RequiredVersion $version
Import-Module PnP.PowerShell -RequiredVersion $version

In Node this would be simply:

"PnP.PowerShell": "<2.0.0" or "1.x.x"

In my experience, this "best compatible" strategy is very common in the industry at large. It allows client software to benefit from non-breaking bug & security fixes, performance improvements, etc. until teams can safely commit the time to upgrade to new major versions. In this way (and others) semver provides the foundation for a useful implicit contract between maintainers and users.

We can no longer use this library under those assumptions. Every new version must now undergo a full review for breaking changes - probably manual review since our script encountered a runtime error, not a failed-to-parse kind of error. The time commitment involved in that will likely prevent us from benefitting from your hard work on the 1.11 version and possibly future versions as well. For the time being we have locked our scripts to use 1.10 and have a low-priority item to revisit as we are able.

Given your (entirely reasonable) SLA we are not expecting any further action from your side on this. I appreciate your candidness, and I also appreciate yours and the team's effort maintaining this library. It has certainly saved us far more time than this incident has lost, and for that we are grateful.

KoenZomers commented 2 years ago

Understood, Steven. We'll extra take it into consideration with future changes. All there's left for me to add here is that I personally would never recommend anyone to follow that practice with regards to versioning, even though you're right, within the NodeJS landscape, it is more common to do it that way. We've seen enough situations where this also goes haywire in that landscape and why they have added locking down to exact specific versions for the entire dependency tree instead. Given that PnP PowerShell is huge (650+ cmdlets) and we're with a very small team of enthusiasts working on these cmdlets, we can never ever fully test every change or addition we do. Things may work in this version and unintentionally break in a next. If you build and test a script against a certain version, use it against that exact version would be my recommendation. Don't try to fix it if it isn't broken. Don't automatically update to a later version, whether it being a minor or patch build only. Just my take on it.

steven-hyland commented 2 years ago

Noticed this is still open - from my perspective it can be closed, nothing else to add and we're clear on the changes.