microsoft / opensource-management-portal

Microsoft's monolithic, opinionated Open Source Management Portal enabling enterprise scale self-service powered by the GitHub API 🏔🧑‍💻🧰
https://jeffwilcox.blog/2019/06/scaling-25k/
MIT License
495 stars 127 forks source link

Improve Microsoft Graph docs on required permissions #161

Open tarkatronic opened 4 years ago

tarkatronic commented 4 years ago

Describe the bug I've managed to get this so that I can log in via AAD. Now, when I attempt to link my GitHub account, the portal is returning a 500 error on the /link url due to Invalid status code: 403 from the backend. I've added a bit of extra debug in and I can see this is coming from the Microsoft Graph API:

{
  error: {
    code: 'Authorization_RequestDenied',
    message: 'Insufficient privileges to complete the operation.',
    innerError: {
      date: '2020-08-03T20:30:06',
      'request-id': '994aeeab-f00a-484b-b591-0d8ef14d21cf'
    }
  }
}

The problem is, I can't suss out what the necessary permissions are. So far, I have granted delegated permissions for:

From what I can tell, this should be more than enough, given that the request is asking for:

?$select=id,mailNickname,userType,displayName,givenName,mail,userPrincipalName

What am I missing from my permissions to get this working properly? Will this even work for a non-MS company?

tarkatronic commented 4 years ago

I believe I have managed to find the answer here, thanks to the Microsoft Graph Explorer. When executing a query against /v1.0/users/<id>/, it shows 15 permissions in use. In actuality, once I de-duped those permissions, it narrowed down to 8.

This does not changed based on what fields you are querying for. So my question now is... do I really need to add all of these permissions?? This seems quite excessive just to grab some information about my own profile. Three of these are for global write access, and 5 of them require admin consent! Is there no simpler way to retrieve this? This level of permission makes this start to look like a security risk.

As a note, accessing /v1.0/me/ requires the same de-duped set of permissions.

jeffwilcox commented 4 years ago

There's def. no reason for any kind of write permission!

tarkatronic commented 4 years ago

I agree! Unfortunately I'm still getting the same error with my permissions set up like this: Screen Shot 2020-08-04 at 5 26 51 PM

I can ask our admins to grant those other permissions, but I really would rather not have to...

jeffwilcox commented 4 years ago

I hear you... permissions can take ages to get approved here, too, if at all...

Do you think you need the graph features? Things it would give you:

Otherwise, we could probably feature flag it out, or find other options... for example, you may have enough info in the AAD passport response to determine if you think they'll work in your directory.

tarkatronic commented 4 years ago

I think this may require a combination of solutions. I can't say for sure whether I will need the graph features, but those two uses you listed sure do sound nice to have.

Regardless, I think a feature flag would be good here. That way, the application will at least work by default. And aside from that, it's probably good to add some documentation on these permission requirements, to make this setup easier for any other users.

Happy to help out here however I can. 😄

ElenaForester commented 3 years ago

I had a similar issue and it was resolved when added Directory.Read.All with Application type image