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

Add support for unconsenting a permission #1844

Closed
5 tasks done
darrelmiller opened this issue Jun 17, 2022 · 18 comments · Fixed by #2063
Closed
5 tasks done

Add support for unconsenting a permission #1844

darrelmiller opened this issue Jun 17, 2022 · 18 comments · Fixed by #2063
Assignees
Labels
Area: Permissions type:enhancement Enhancement request targeting an existing experience
Milestone

Comments

@darrelmiller
Copy link
Contributor

darrelmiller commented Jun 17, 2022

Is your feature request related to a problem? Please describe.
It is currently not possible from within Graph Explorer to remove a previously consented scope to test scopes with lower privileges.

  • Implement revoke consent
  • Enable tenant admins to revoke consent for tenant-wide permissions
  • Add a column for consentType
  • UI enhancements: Make 'Admin consent required' tab title wrap on small screens, Disable Unconsent button for users with no required permissions
  • Add info icon to Unconsent button and show consent-type column regardless of token being present
@ghost ghost added ToTriage and removed ToTriage labels Jun 17, 2022
@adhiambovivian adhiambovivian added type:enhancement Enhancement request targeting an existing experience Area: Permissions labels Jun 22, 2022
@RabebOthmani RabebOthmani added this to the Aug-2022 milestone Aug 3, 2022
@adhiambovivian
Copy link

@RabebOthmani this work item needs a UI/UX design wireframe or the other option if for us to prototype and review together

@RabebOthmani
Copy link

@adhiambovivian my plan is to do minimal UI/UX changes due to resources. We will rely on the current UI and make necessary changes on it.

@RabebOthmani
Copy link

@darrelmiller @adhiambovivian @MaryannGitonga @gavinbarron for the permissions tab, here's what I'm thinking. Please remember that the goal here is not to reinvent the UI and do minimum required work there.
This is the current tab:
permissions tab old
I'm thinking:
permissions tab proposal
so it would look something like this
permissions tab new

@RabebOthmani
Copy link

RabebOthmani commented Aug 11, 2022

@MaryannGitonga as for the actual implementation, what we need is to update oAuth2PermissionGrant to remove items (permissions in this case).
Forgot the link doh https://docs.microsoft.com/en-us/graph/api/oauth2permissiongrant-delete?view=graph-rest-1.0&tabs=http
@adhiambovivian I'd suggest pairing Maryann with Evans or Elinor. Permissions are tricky to understand, let us give her the support she needs :)

@RabebOthmani
Copy link

@darrelmiller @gavinbarron @adhiambovivian @MaryannGitonga for the Permissions list under the user's profile. I captured some challenges with the current experience

Permissions.list.mp4

Similar to the permissions tab, is there any value of showing a count of the permissions selected?
number of permissions

After some thought, do we want to encourage users to select a group of permissions? Isn't that against the recommendation to consent the least privileged whenever possible? @darrelmiller I'd love your opinion on this.
My suggestions would be to remove the checkboxes, keep the list grouping and apply similar modifications to what we will apply on permissions tab (consent/unconsent button , yes/no admin etc)

@RabebOthmani
Copy link

How are we filtering the list of permissions cc @adhiambovivian
NA permissions
Family permission is hidden on Graph but it appears on the list. It shouldn't
family permissions

@Onokaev
Copy link
Contributor

Onokaev commented Aug 29, 2022

While working on this, we noted some interesting things:
GE requires the User.Read permission to make bare-minimum calls. What do we do in case a user unconsents to User.Read permission? Do we log them out? Do we restrict the ability to consent to such a permission?

Unconsenting requires the following permissions:
image
These permissions require admin consent before a user can consent to them. What do we do in such a scenario?

@Onokaev Onokaev self-assigned this Aug 30, 2022
@gavinbarron
Copy link
Member

I'm extrapolating a little here but it sounds like at an absolute minimum the user needs to have User.Read for some calls and there is no lower valid permission that can be consented to.
This is coupled with the fact that to unconsent to a permission Directory.Read.All is required.

Can we prevent these permissions from being unconsented and add a call out in the UI that User.Read and Directory.Read.All are the lowest permissions available to still have the GE application functional?

@MaryannGitonga
Copy link
Contributor

@gavinbarron in that case, should Directory.Read.All (and DelegatedPermissionGrant.ReadWrite.All which is another permission required to enable unconsenting to permissions) be added to the list of default scopes that a user is consented to first off, on signing into GE?

@gavinbarron
Copy link
Member

Great question, and I'm sorry to answer with another question.
What are the scopes we request on the initial sign-in at present? Are there permissions that require admin consent for a user to sign in? (My gut is that we do need admin consent being a multi-tenant app).

I'm hesitant to ask for those permissions until we absolutely need them, such as when a user wants to revoke consent for a scope just due to them having such wide reach.

As a good practice we should ask only for the minimum permissions necessary initially and then leverage incremental consent as needed.

@MaryannGitonga
Copy link
Contributor

MaryannGitonga commented Sep 2, 2022

@gavinbarron These are the default permissions are openid, profile and User.Read.

The two permissions required for unconsenting require admin consent unlike the current default permissions. With regard to and appreciation of the principle of least privilege access, then the best option would be to let the user consent to them when they need to revoke a permission.

@gavinbarron
Copy link
Member

I agree, thank you.

@RabebOthmani
Copy link

@Onokaev per our conversation today during the sync:

  • Can we please ensure that the requirements stated on this thread are followed (text content, UI, etc)
  • Consent to permissions modal should be consistent with the permissions tab. Action for me: Consult with the UX team to come up with a good filtering way.
  • Tested and the filtering is still not quite correct

@RabebOthmani
Copy link

@Onokaev the consent/revoke button is the most important functionality on this screen and yet I can't see it properly. In comparaison the description is taking too much horizontal space.
@gavinbarron could you please assist us with this . Thank you :)
permissions layout

@Onokaev
Copy link
Contributor

Onokaev commented Oct 6, 2022

@RabebOthmani what did you try to filter? I just tested and it's working on my end
I have made the changes to the text content, so that's covered. I have also done a quick fix on the length of the description. Let me know how that looks on your screen :)

@gavinbarron
Copy link
Member

@RabebOthmani happy to lend a hand here, although it looks like @Onokaev has added some text wrapping and a fix to ensure that the Consent/Unconsent button is not obscured by the scrollbar.
Not sure that there's much value for me to add on this.

@Onokaev
Copy link
Contributor

Onokaev commented Oct 11, 2022

How are we filtering the list of permissions cc @adhiambovivian NA permissions Family permission is hidden on Graph but it appears on the list. It shouldn't family permissions

Hey @RabebOthmani. This filtering requires some work on the DevX API repo

@Onokaev
Copy link
Contributor

Onokaev commented Nov 3, 2022

Ticket tracking filtering of permissions: microsoftgraph/microsoft-graph-devx-api#1232

@Onokaev Onokaev closed this as completed Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Permissions type:enhancement Enhancement request targeting an existing experience
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants