Skip to content

Conversation

pranavkm
Copy link
Contributor

Contributes to #5680

@ghost
Copy link

ghost commented Jun 16, 2020

Hello @pranavkm!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 60 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@pranavkm pranavkm force-pushed the prkrishn/nullable-authz branch from 74de638 to 86fccf3 Compare June 16, 2020 04:31
@pranavkm pranavkm marked this pull request as ready for review June 16, 2020 15:39
@pranavkm pranavkm requested a review from Tratcher as a code owner June 16, 2020 15:39
@pranavkm pranavkm requested a review from HaoK June 16, 2020 15:39
@Pilchie Pilchie added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jun 16, 2020
@@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
Copy link
Member

Choose a reason for hiding this comment

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

nit: is this using needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Will clean up in a follow up

@@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Diagnostics.CodeAnalysis;
Copy link
Member

Choose a reason for hiding this comment

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

nit: needed?

@@ -37,7 +37,16 @@ public static IServiceCollection AddAuthorizationPolicyEvaluator(this IServiceCo
/// <param name="services">The <see cref="IServiceCollection" /> to add services to.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static IServiceCollection AddAuthorization(this IServiceCollection services)
=> services.AddAuthorization(configure: null);
{
Copy link
Member

Choose a reason for hiding this comment

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

Is there no clean way to share chain these methods anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following from MVC where it does an explicit null check if you call the overload that takes an option. That unfortunately necessitates having separate code paths.

@ghost ghost merged commit f3b370c into master Jun 18, 2020
@ghost ghost deleted the prkrishn/nullable-authz branch June 18, 2020 23:43
Copy link
Contributor Author

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

Thanks for the review. I'll clean up the extra attributez

@@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Will clean up in a follow up

@@ -37,7 +37,16 @@ public static IServiceCollection AddAuthorizationPolicyEvaluator(this IServiceCo
/// <param name="services">The <see cref="IServiceCollection" /> to add services to.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static IServiceCollection AddAuthorization(this IServiceCollection services)
=> services.AddAuthorization(configure: null);
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following from MVC where it does an explicit null check if you call the overload that takes an option. That unfortunately necessitates having separate code paths.

@pranavkm pranavkm added this to the 5.0.0-preview7 milestone Jun 19, 2020
services.Configure(configure);
}

services.Configure(configure);
Copy link
Member

Choose a reason for hiding this comment

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

This can null ref now if the caller hasn't turned on annotations.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants