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

🐞 Bug report: permissions granted to the toolkit are wider than documented #302

Closed
michaelmaillot opened this issue Sep 18, 2024 · 7 comments Β· Fixed by #305
Closed
Assignees
Labels
🐞 bug Something isn't working πŸ‘¨β€πŸ’»work in progress I am working on it

Comments

@michaelmaillot
Copy link

⭐ Priority

(Low)☹️ Something is a little off

πŸ“ Describe the bug

Hello,

When using the Toolkit to register (automatically) the Entra ID app which is used for the CLI, there are more API permissions granted than the ones documented.

Permissions expected to be granted according to the documentation:

https://github.com/pnp/vscode-viva/blob/main/src/webview/view/components/forms/entraAppReg/RegisterEntraAppRegView.tsx#L92

image

Permissions granted in the app:

image

Some permissions such as Directory.ReadWrite.All can be a blocker in some organizations.

πŸ‘£ Steps To Reproduce

  • Install the SPFx Toolkit
  • Sign In to Microsoft 365
  • Select th option "Create a new app registration"
  • Select "Automatically"
  • Click on "Create the Entra App Registration"
  • Valdiate the registration through the Toolkit / browser
  • Go to the "SPFx Toolkit" Entra ID App, then the "API permissions"

πŸ“œ Expected behavior

Permissions granted in the documentation should reflect the effective ones.

πŸ“· Screenshots

No response

❓SharePoint Framework Toolkit version

4.0.0

❓Node.js version

18.19.0

πŸ€” Additional context

No response

@michaelmaillot michaelmaillot added the 🐞 bug Something isn't working label Sep 18, 2024
@Adam-it Adam-it self-assigned this Sep 18, 2024
@Adam-it
Copy link
Contributor

Adam-it commented Sep 18, 2024

@michaelmaillot thanks for pointing it out.
I double checked and in code we actually set

 'https://graph.windows.net/Directory.AccessAsUser.All',
  'https://management.azure.com/user_impersonation',
  'https://graph.microsoft.com/User.Read',
  'https://graph.microsoft.com/AppCatalog.ReadWrite.All',
  'https://graph.microsoft.com/AuditLog.Read.All',
  'https://graph.microsoft.com/SecurityEvents.Read.All',
  'https://graph.microsoft.com/ServiceHealth.Read.All',
  'https://graph.microsoft.com/ServiceMessage.Read.All',
  'https://graph.microsoft.com/Sites.Read.All',
  'https://graph.microsoft.com/Directory.AccessAsUser.All',
  'https://graph.microsoft.com/Directory.ReadWrite.All',
  'https://manage.office.com/ActivityFeed.Read',
  'https://manage.office.com/ServiceHealth.Read',
  'https://microsoft.sharepoint-df.com/AllSites.FullControl',
  'https://microsoft.sharepoint-df.com/User.ReadWrite.All'

it seems in documentation I missed adding info about πŸ€¦β€β™‚οΈ

  'https://graph.windows.net/Directory.AccessAsUser.All',
  'https://management.azure.com/user_impersonation',

I will need to include it in the documentation.
Thank you for pointing it out.
You Rock 🀩

@michaelmaillot
Copy link
Author

michaelmaillot commented Sep 18, 2024

If I'm not mistaken, there are also the following ones which are missing in the docs:

'https://graph.microsoft.com/Directory.AccessAsUser.All',
'https://graph.microsoft.com/Directory.ReadWrite.All'

Regarding the "Register Entra App" page in the extension:

image

@Adam-it
Copy link
Contributor

Adam-it commented Sep 18, 2024

yes, that as well πŸ‘. Thanks for the double check

@Adam-it
Copy link
Contributor

Adam-it commented Sep 18, 2024

@michaelmaillot would you like to take it up and contribute to this repo? Or you would rather leave it for me?
if needed I may point you in the places where we need to correct it.

@Adam-it
Copy link
Contributor

Adam-it commented Sep 21, 2024

I'll take it.
Plan:

  • Fixup wiki
  • fixup RegisterEntraAppRegView.tsx

@Adam-it Adam-it linked a pull request Sep 21, 2024 that will close this issue
2 tasks
Adam-it added a commit that referenced this issue Sep 21, 2024
)

## 🎯 Aim

The aim is to provide fixup to recently found errors and issues

## βœ… What was done

- [X] Adds missing scopes
- [X] Adds error handling to init

## πŸ”— Related issue

Closes: #302,  #304
@Adam-it
Copy link
Contributor

Adam-it commented Sep 21, 2024

Done βœ…
This will be part of an upcoming pre-release and soon after a new minor release

@Adam-it Adam-it closed this as completed Sep 21, 2024
@michaelmaillot
Copy link
Author

Woopsie, sorry I didn't see your proposal! Thanks for having handled this one.

I'll try to pick another one in the issue list another time πŸ€—

Adam-it added a commit that referenced this issue Sep 28, 2024
)

## 🎯 Aim

The aim is to provide fixup to recently found errors and issues

## βœ… What was done

- [X] Adds missing scopes
- [X] Adds error handling to init

## πŸ”— Related issue

Closes: #302,  #304
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working πŸ‘¨β€πŸ’»work in progress I am working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants