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

feat: add requirement for denying guest users #2179

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

h3rmanj
Copy link
Contributor

@h3rmanj h3rmanj commented Apr 3, 2023

As a developer, I expect to be able to easily allow all members of a tenant to access my application, without needing to set up roles and manual assignment.

However, Azure AD allows for guest users, that applications like Teams rely on. These guest users will then by default have access to applications that doesn't have any roles, but still only should be accessible by tenant members.

This PR introduces the DenyGuests extension to AuthorizationPolicyBuilder, which easily lets the developer deny guests users access to their applications with policies.

I think that many enterprise application would want this behavior by default, and without it, the application would be exposed to some vulnerabilities.

//cc @loekensgard @JonasKs

/// Implements an <see cref="IAuthorizationRequirement"/>
/// which requires the current user to be a member of the tenant.
/// </summary>
public class DenyGuestsAuthorizationRequirement : IAuthorizationRequirement
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not locked on the naming of this. I see many requirements are prefixed with Require, but RequireTenantMembers or RequireNonGuests sounds weird and isn't as explicit in what it tries to achieve.

@loekensgard
Copy link

@microsoft-github-policy-service agree

@loekensgard loekensgard force-pushed the deny-guests-requirement branch 2 times, most recently from 8a2e238 to 1b0f918 Compare April 4, 2023 12:41
Signed-off-by: Thorstein Løkensgard <[email protected]>
@loekensgard loekensgard force-pushed the deny-guests-requirement branch from 1b0f918 to 1ae9fe3 Compare April 4, 2023 12:50
@jmprieur
Copy link
Collaborator

thanks @h3rmanj
Adding more authorization is definitively something we want to do, but we are thinking of doing it a bit differently
cc: @GeoK

@h3rmanj
Copy link
Contributor Author

h3rmanj commented Apr 11, 2023

Mind sharing your thoughts? This is how we've done it internally for now, but we'd love to hear the plans for actual implementation in the SDK or even Azure itself.

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