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

On propertybag set, remove the error that is thrown if a site has NoScript enabled #6459

Open
martinlingstuyl opened this issue Nov 1, 2024 · 3 comments
Labels
enhancement needs peer review Needs second pair of eyes to review the spec or PR

Comments

@martinlingstuyl
Copy link
Contributor

martinlingstuyl commented Nov 1, 2024

Currently, in our spo properybag set command, we check if a site has NoScript enabled, and throw an error if it does.

However, as explained in 6458 this restriction can be lifted if you configure a tenant-wide property.

I'm thinking we should remove the thrown error, so that the command can just be executed. If it is a NoScript site, and the setting has not been configured, it will throw an error anyway. But that's okay I guess.

I'm talking about this code in propertybag-set.ts:

// Check if web no script enabled or not
// Cannot set property bag value if no script is enabled
const isNoScriptSite = await this.isNoScriptSite(identityResp, args.options, logger);

if (isNoScriptSite) {
  throw 'Site has NoScript enabled, and setting property bag values is not supported';
}
@martinlingstuyl martinlingstuyl added enhancement needs peer review Needs second pair of eyes to review the spec or PR labels Nov 1, 2024
@Adam-it
Copy link
Member

Adam-it commented Nov 4, 2024

@martinlingstuyl did you maybe check what would be the error message? I hope it is something that makes sense 😉

@martinlingstuyl
Copy link
Contributor Author

Not yet no, I'll check it out 👍

@martinlingstuyl
Copy link
Contributor Author

I checked @Adam-it, you'll get an access denied:

Error: Access is denied. (Exception from HRESULT: 0x80070005 (E_ACCESSDENIED))

So that's not very clear. Should we show a warning above or below the error? Something like It might be that NoScript is enabled and the property bag has not been enabled to function on NoScript sites.

The downside is that we'd need a lot of explanatory text, while the error might not be due to NoScript at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs peer review Needs second pair of eyes to review the spec or PR
Projects
None yet
Development

No branches or pull requests

2 participants