search

pnp / PnP-PowerShell

SharePoint PnP PowerShell CmdLets
https://pnp.github.io/powershell
Other
987 stars 665 forks source link

Clarify documentation to mention requirement of connecting to -admin url #2742

Closed AbhishekGarg closed 4 years ago

AbhishekGarg commented 4 years ago

Reporting an Issue or Missing Feature

Add-PnPSiteCollectionAppCatalog does not work by usual method of connecting to target site via Connect-PnPOnline and executing the "Add" cmdlet. It needs connection to the tenant -admin url, noted in this comment for #2149.

As this is a departure from the usual method, and not something which is obvious, it should be mentioned in cmdlet doc page, rather than only in the full documentation.

OP in #2611 looks like having the same confusion.

Expected behavior

Actual behavior

Steps to reproduce behavior

Which version of the PnP-PowerShell Cmdlets are you using?

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

2.25.1804.1

How did you install the PnP-PowerShell Cmdlets?

ghost commented 4 years ago

Thank you for reporting this issue. We will be triaging your incoming issue as soon as possible.

KoenZomers commented 4 years ago

Fair point @AbhishekGarg. Thanks for your feedback here.

The Add-PnPSiteCollectionAppCatalog cmdlet actually inherits from PnPAdminCmdlet to indicate it requires access to the SharePoint Tenant Admin site. Now I understand for non developers, this is not easy to find. It does contain logic, however, that it tries to self elevate to the SP Admin center site on the background, if such a cmdlet is being run. Apparently in your scenario that seems to fail, but normally it should not have to matter.

What I'm going to propose is that for all cmdlets inheriting from PnPAdminCmdlet is that we add an extra line in the Get-Help documentation generation that we list in there explicitly that the cmdlet needs access to the SharePoint Admin Center site, like so:

image

Do you think that will make it sufficiently clear what is needed here?

KoenZomers commented 4 years ago

PR with this change: https://github.com/pnp/PnP-PowerShell/pull/2751

AbhishekGarg commented 4 years ago

The Add-PnPSiteCollectionAppCatalog cmdlet actually inherits from PnPAdminCmdlet to indicate it requires access to the SharePoint Tenant Admin site. Now I understand for non developers, this is not easy to find. It does contain logic, however, that it tries to self elevate to the SP Admin center site on the background, if such a cmdlet is being run. Apparently in your scenario that seems to fail, but normally it should not have to matter.

IMHO, it is not even easy for devs to find, because no one would be looking at the code first before using the cmdlet, to check if it inherits from PnPAdminCmdlet. It should be in documentation. Thank you for proposing the change :)

It is definitely strange that it fails to elevate in my case, because my account HAS admin rights.

What I'm going to propose is that for all cmdlets inheriting from PnPAdminCmdlet is that we add an extra line in the Get-Help documentation generation that we list in there explicitly that the cmdlet needs access to the SharePoint Admin Center site,

Do you think that will make it sufficiently clear what is needed here?

I'll request to make it more clear, by stating that 'Connection' to admin site is required before running the cmdlet (that obviously means that access is required). Only stating 'access is required' doesn't imply connection, I think.

KoenZomers commented 4 years ago

I was struggling with the wording a bit on that as theoretically it should not be mandatory to connect to -admin as I mentioned. What method of connecting are you using? Perhaps we can find out why it doesn't elevate to admin from that.

AbhishekGarg commented 4 years ago

I connect to the target site using Connect-PnPOnline (UseWebLogin for MFA). Then, trying to run Add-PnPSiteCollectionAppCatalog fails with a 403 error.

Connecting to -admin url explicitly (without connecting to target site) and then using the appcatalog cmdlet on target site succeeds.

KoenZomers commented 4 years ago

I can reproduce it here. Let me see if there would be some way to fix that. image

KoenZomers commented 4 years ago

I see it's because the fedAuth cookie doesn't get cloned along to the -admin context it tries to create. Trying to see if I can get it cloned over as well. At the very least I can throw an error that it failed to elevate to -admin and a manual connect to -admin should take place instead.

AbhishekGarg commented 4 years ago

Very nice find! I would be very interested in the outcome.

KoenZomers commented 4 years ago

Found the reason why the elevation fails. Basically when using Connect-PnPOnline -Url https://tenant.sharepoint.com -UseWebLogin, it gathers and sets the FedAuth cookie for tenant.sharepoint.com only. When running a PnPAdminCmdlet on that connection, such as Get-PnPTenant, it will try to clone the ClientContext to a context for tenant-admin.sharepoint.com, but due to the complexity and weirdness of how the ClientContext has been created within CSOM, we don't have access to the cookiecollection on an existing ClientContext so we cannot add the cookie to it during cloning. Filed PR https://github.com/pnp/PnP-Sites-Core/pull/2687 with PnP Sites Core to immediately on connect to already set the FedAuth cookie for both tenant.sharepoint.com and tenant-admin.sharepoint.com domains so the auto elevation in the PnPAdminCmdlets works again. That together with the extra clarification in the Get-Help should resolve this issue.

AbhishekGarg commented 4 years ago

Thank you very much for this :)

I have one question here. If I understood correctly, this change will always set the cookie for both tenant and tenant.admin domains.

Wouldn't it throw an error in case the account doesn't have SharePoint admin permissions? Asking this because many cmdlets do not require connecting to -admin through PnPAdminCmdlets, as they are meant to work only on the particular site collection.

KoenZomers commented 4 years ago

You're welcome! It will indeed always set the cookie, but that shouldn't be a problem. Look at it as a keychain. With this change, when you log in, we put a key on your keychain for SPO and an extra key for SPO-Admin. At the point of putting this extra key on the keychain, it doesn't mean the key actually needs to fit on the SPO-Admin lock. Only when you would initiate a PnPAdminCmdlet, it will take the SPO-Admin key from the keychain and try if it fits on the SPO-Admin door. If it doesn't, it will throw a 401 unauthorized exception, which is perfectly fine.

AbhishekGarg commented 4 years ago

Wonderful example, understood perfectly :-)