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

SPFx as spyware - updates #1900

Merged
merged 29 commits into from
Nov 26, 2024
Merged

SPFx as spyware - updates #1900

merged 29 commits into from
Nov 26, 2024

Conversation

kkazala
Copy link
Contributor

@kkazala kkazala commented Aug 13, 2024

Category

  • Content fix
  • New article

Contents of the Pull Request

Important This PR has a dependency on

2nd article in SPFx as spyware series, covering

  • PS script to review spfx extensions and requested permissions, and API permissions granted to the SP service principal
  • adding Application Insights tracking to all SPO modern sites, analyzing results and alerts

Reference added to the 1st article from the series

And... tags added to the old Power Platform extensibility articles. I already forgot about this change and as a result, everything in one PR. I'm sorry, can we leave it like that?
Spaces added automatically by "Prettier", hope it's ok?

Guidance

@kkazala kkazala changed the title tags added SPFx as spyware - part II Aug 16, 2024
@VesaJuvonen
Copy link
Contributor

VesaJuvonen commented Aug 16, 2024

These are great articles to educate the ecosystem - however - we seem to be missing the technical detail that to be able to install or start using SPFx solutions - they have to be deployed and installed by administrators.

Would absolutely include that detail already to the part 1 to avoid confusion - as now it gives an impression that anyone can start using SPFx solutions - which of course it not the case... this was the key differentiator vs the script editor web part we had in the classic experience - where admin and governance for the extensibility was not required... with the SPFx - any solution which is installed on the tenant has to be explicitly approved by the tenant administrator - or the app administrator if responsibility is delegated forward - so there's additional governance and admin controls by design for the IT - which also can control the usage of the SPFx solutions from one central location. This quite important detail seems to be missing at least from the part 1 on this series.

Worth noticing is also that the SPFx solutions have the central coordination and governance for the IT administrators - which is a bit different than the Power Platform side of the story. Both have advantages and disadvantages for sure. Power Platform improves all the time though for sure on these areas as well - which is great.

As said - otherwise it's great that we educate the customers on the different considerations, but would love to get above details noted to avoid confusion.

@kkazala
Copy link
Contributor Author

kkazala commented Aug 16, 2024

Thx @VesaJuvonen for the feedback. I will update the articles to elaborate more on roles and responsibilities.
With the site-level app catalog, however, once enabled, site owners can install the solutions themselves. The attack surface is smaller, but not negligible. That's why I mentioned that "Security Awareness: If site level catalogs are enabled in the tenant, educate the site owners about the risks associated with installing and using third-party solutions."

I see your point, it's at the very end and could be easily missed.
Will make an update soon :)

thx again

@kkazala
Copy link
Contributor Author

kkazala commented Aug 18, 2024

@VesaJuvonen Please have a look. I think that these two articles in the series now paint a more complete picture :)

Still waiting for the second PR:

Please do not approve before the pnp/sp-dev-fx-webparts#5203 is merged

@VesaJuvonen
Copy link
Contributor

"With the site-level app catalog, however, once enabled, site owners can install the solutions themselves. "

However - you need to be a tenant administrator to create site-level app catalog, providing IT full governance and control on allowing site owners to do this. There's no way end users can start using SPFx solutions without control and governance from the administrators.

In general site-level app catalogs only have really minimal use currently worldwide for these exact reasons - as it would allow the site collection owners to accidently cause issues - after the usage has been allowed by the administrators.

@kkazala
Copy link
Contributor Author

kkazala commented Aug 19, 2024

@VesaJuvonen
"However - you need to be a tenant administrator to create site-level app catalog,"- SharePoint Admin is enough
"In general site-level app catalogs only have really minimal use currently worldwide" - we currently have 10 sites with site-level app catalog.

Not sure what you'd like me to correct here. How about I quote you? This would be fantastic, to have your statement here.

It is not my goal to claim that SPO securing is bad. It isn't. But after hearing things like:

  • "we only allow delegated permissions" - meaning we have it covered and nothing to worry about
  • "we are now trying to find out which spfx solution has which permissions"- rrrrightt
  • "ok, so I'm just going to search for these 2 lines of code [retrieving access token]"- ehmm.... yeah

I decided we really have to discuss this topic. It's not about vulnerabilities in SPFx, SPO or M365. We are the vulnerability

@kkazala kkazala changed the title SPFx as spyware - part II SPFx as spyware - updates Aug 20, 2024
@kkazala
Copy link
Contributor Author

kkazala commented Aug 20, 2024

Removed all the other changes from the PR to isolate the topic

@VesaJuvonen
Copy link
Contributor

Thx @kkazala on the updates. This is incredibly important topic which has to be for sure discussed so that our customers and partners understand what they are consenting and what the solutions can do within their environment. Transparency is for sure the key and liked following callout a lot - "This approval process is far more than just a formality and must be taken extremely seriously. The decision to approve a solution can have significant implications for the security of the entire tenant."

Edits are definitely great and provide more insights on the process - as initially the admin role was not discussed in the article - and it simply called out that solutions can access information - which is not wrong - but it can only happen if administrators approve the solutions. That's really the key on the modern SharePoint experiences - with SPFx vs the classic SharePoint with script editor web part - which technically allowed these kind of things to be done WITHOUT admin control - as any site owner could copy script and place that on the site.

Since classic SharePoint is still supported (yes - amazing it still is) - we have introduced the default settings with the noscript setting - which by default blocks end users to use script editor web part in classic SharePoint. So - there's at least recommendations for this from Microsoft - even though some customers are still using classic and they allow script editor web part usage - which is a clear risk.

On the PR - definitely ok to get merged - however it is now referencing the part 2 article - which was removed from this PR, so would not merge this yet... So - would suggest to get the part 2 back - or edit this to remove that link for now - and then submit a new PR when you are ready with that.

Thank you for helping to increase the awareness of the security topics - critical for the ecosystem to understand 🙏

@kkazala
Copy link
Contributor Author

kkazala commented Aug 26, 2024

@VesaJuvonen Fantastic, I'm happy you like the updates :)
I re-added the Part II now, and also (in Part II) included additional suggestion for automated cleanup of unused API permissions.
I'm referencing scripts from my gist now, because I just did a pnp/script-samples#732 to add them to the official samples, but i know it takes few days and I would like to push the update to the part I as as soon as possible.

Do you know how long it takes to approve webpart samples?
I'm suggesting tenant-wide deployment of Application Customizer solution for tracking external API calls and I am now linking the sample from my GitHub account, because the pnp/sp-dev-fx-webparts#5203 is still pending

thank you for all your help and support =)

@VesaJuvonen
Copy link
Contributor

Just to follow up on here - you are not forgotten @kkazala.

Your PR in the SPFx web parts should be processed this weekend and then we can merge these updates in early next week. Thanks for your patience.

@kkazala
Copy link
Contributor Author

kkazala commented Aug 30, 2024

Awesome, thx @VesaJuvonen for the heads up.
Could you give me until Tuesday/Wednesday to update the reference to the web part? And maybe I'll add info about revoking refresh tokens (https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens) once I'm at it.
:)

@kkazala
Copy link
Contributor Author

kkazala commented Sep 4, 2024

@VesaJuvonen I updated the links to the scripts and the webpart, so let's merge, if it's OK with you?
Otherwise I will keep updating it and will never finish :D

@kkazala
Copy link
Contributor Author

kkazala commented Sep 14, 2024

@VesaJuvonen do you think we could merge it some time soon? :)

@Adam-it
Copy link
Member

Adam-it commented Nov 26, 2024

@VesaJuvonen do you think we could merge it some time soon? :)

ooops I think this was missed for some time now 😮
@kkazala sorry for that. I will try to ping @VesaJuvonen on this and merge it ASAP.

Updating date for the new article so that it does not get buried
@VesaJuvonen
Copy link
Contributor

@kkazala I'm so so so sorry for missing this completely as it dropped from my radar - I get way too many GitHub comments in notification/email format, which makes it impossible to follow up on them, but it's a bad excuse. I should have ensured that we get our discussion finalized and published. Thanks @Adam-it for the direct message on getting this processed 🙏

I've now updated the date for the part 2 article - for today - so that it will not be buried on the history.

@VesaJuvonen VesaJuvonen merged commit 946bcd0 into pnp:main Nov 26, 2024
2 of 3 checks passed
@Adam-it
Copy link
Member

Adam-it commented Nov 26, 2024

@VesaJuvonen thanks a lot for your help 👍
This will also get a mention on the upcoming Viva Community Call
@kkazala did you already opt-in our recognition program?
https://pnp.github.io/recognitionprogram/
You should definitely do that haven't already and get the Blog Author badge

@kkazala
Copy link
Contributor Author

kkazala commented Dec 2, 2024

Thank you @Adam-it for keeping an eye on it, and @VesaJuvonen for updating and merging, and for your kind explanations =)
I fully understand how it is... a bit of "Everything Everywhere All at Once", isn't it ;)

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.

3 participants