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

Resolves Issue #440 #441

Closed
wants to merge 2 commits into
base: authorization
from

Conversation

Projects
None yet
3 participants
@michaelmccord
Copy link

michaelmccord commented Dec 31, 2018

  1. Add a default constructor to AuthorizeDirective
  2. When no policy is specified on the directive usage, use the default policy.
  3. Return an error if no policy could be found.
  4. Remove a level of braces in AuthorizeDirectiveType.AuthorizeAsync where possible
1. Add a default constructor to AuthorizeDirective
2. When no policy is specified on the directive usage, use the default policy.
3. Return an error if no policy could be found.
4. Remove a level of braces in AuthorizeDirectiveType.AuthorizeAsync where possible
@michaelstaib

This comment has been minimized.

Copy link
Collaborator

michaelstaib commented Dec 31, 2018

@michaelmccord thanks for your work.

@michaelstaib michaelstaib requested a review from rstaib Dec 31, 2018

@michaelstaib michaelstaib added this to the 0.7.0 milestone Dec 31, 2018

@michaelstaib

This comment has been minimized.

Copy link
Collaborator

michaelstaib commented Dec 31, 2018

@michaelmccord I have left some comments. Most are just style annotations. We have sonar rules to enforce that the braces are always be used. It would be great if you could fix the issues. I will write some at tests around this.

@michaelstaib michaelstaib changed the base branch from master to authorization Dec 31, 2018

@michaelstaib

This comment has been minimized.

Copy link
Collaborator

michaelstaib commented Dec 31, 2018

@rstaib @michaelmccord I have changed the base branch and will add the unit tests to this one.

@michaelmccord

This comment has been minimized.

Copy link

michaelmccord commented Jan 1, 2019

@michaelmccord I have left some comments. Most are just style annotations. We have sonar rules to enforce that the braces are always be used. It would be great if you could fix the issues. I will write some at tests around this.

I am unable to see your comments? Permissions issue?

// are applicable only to AspNet Core or not. So, I'm keeping this in but gating
// it for AspNetClassic until someone can confirm it's okay to remove this for both
// AspNetClassic and AspNetCore.
#if ASPNETCLASSIC

This comment has been minimized.

@michaelstaib

michaelstaib Jan 1, 2019

Collaborator

@rstaib can you look at this?

This comment has been minimized.

@rstaib

rstaib Jan 1, 2019

Collaborator

Makes no sense because we are already in the ASPNETCORE branch and asking if ASPNETCLASSIC. This code will never be executed. Neither in ASPNETCORE nor in ASPNETCLASSIC. Just remove this #if statement.

Show resolved Hide resolved src/Server/AspNetCore.Authorization/AuthorizeDirectiveType.cs Outdated
Show resolved Hide resolved src/Server/AspNetCore.Authorization/AuthorizeDirectiveType.cs

}

private static QueryError BuildUnauthorizedError(IDirectiveContext context, bool policyNotfound = false)

This comment has been minimized.

@michaelstaib

michaelstaib Jan 1, 2019

Collaborator

@michaelmccord @rstaib I think we should return one or the other. So if the policy is not found return that the policy was not found. This way we know that there is an issue with the authorize annotation. If the user is not allowed to access the resource then return this message. This way it will be more clear what the problem is.

This comment has been minimized.

@michaelmccord

michaelmccord Jan 1, 2019

My thinking behind this message was that in either scenario the user is not authorized it's just that in the case where the @authorize directive is configured incorrectly there's additional reasoning beyond simply being unauthorized.

This comment has been minimized.

@michaelmccord

michaelmccord Jan 1, 2019

If clarity is an issue, my suggestion would actually be to beef up the other case. Something like:
"The current user is not authorized to access this resource. The user did not meet the requirements of the authorization policy."

This comment has been minimized.

@michaelstaib

michaelstaib Jan 1, 2019

Collaborator

But is it not a configuration error that the policy is wrong. So, it is basically by accident that the user cannot access the resource and by that it is more like a processing error. We could also throw an exception in that case. Do we even want to give the enduser of the api the information that something is misconfigured?

This comment has been minimized.

@rstaib

rstaib Jan 1, 2019

Collaborator

Also merging error messages could get confusing for maintainers. It is just harder to read as having specific error messages for each specific case. We would like to avoid complexity in general where ever we can.

@michaelstaib

This comment has been minimized.

Copy link
Collaborator

michaelstaib commented Jan 1, 2019

can you see the now?

@michaelmccord

This comment has been minimized.

Copy link

michaelmccord commented Jan 1, 2019

@michaelstaib the style issues have been addressed.

@rstaib
Copy link
Collaborator

rstaib left a comment

First of all, thank you for your contribution. I added a few comments mostly about code style. By the way sorry for being to picky but we kinda like our code :-P

// are applicable only to AspNet Core or not. So, I'm keeping this in but gating
// it for AspNetClassic until someone can confirm it's okay to remove this for both
// AspNetClassic and AspNetCore.
#if ASPNETCLASSIC

This comment has been minimized.

@rstaib

rstaib Jan 1, 2019

Collaborator

Makes no sense because we are already in the ASPNETCORE branch and asking if ASPNETCLASSIC. This code will never be executed. Neither in ASPNETCORE nor in ASPNETCLASSIC. Just remove this #if statement.


}

private static QueryError BuildUnauthorizedError(IDirectiveContext context, bool policyNotfound = false)

This comment has been minimized.

@rstaib

rstaib Jan 1, 2019

Collaborator

Also merging error messages could get confusing for maintainers. It is just harder to read as having specific error messages for each specific case. We would like to avoid complexity in general where ever we can.

context.Result = BuildUnauthorizedError(context, true);
return;
}

This comment has been minimized.

@rstaib

rstaib Jan 1, 2019

Collaborator

We would like to avoid unnecessary empty lines here. One empty line would be enough.


#if !ASPNETCLASSIC
if (allowed && !string.IsNullOrEmpty(directive.Policy))

This comment has been minimized.

@rstaib

rstaib Jan 1, 2019

Collaborator

We would like to avoid unnecessary empty lines here.

}

This comment has been minimized.

@rstaib

rstaib Jan 1, 2019

Collaborator

We would like to avoid unnecessary empty lines here.

}

#endif

This comment has been minimized.

@rstaib

rstaib Jan 1, 2019

Collaborator

Would be nice to have one empty line as separation between #endif and await ....

#endif
await next(context);

This comment has been minimized.

@rstaib

rstaib Jan 1, 2019

Collaborator

We would like to avoid unnecessary empty lines here.

@michaelstaib

This comment has been minimized.

Copy link
Collaborator

michaelstaib commented Jan 7, 2019

Hi @michaelmccord,

can push the style fixes that @rstaib stated. After this we could merge this one.

@michaelstaib

This comment has been minimized.

Copy link
Collaborator

michaelstaib commented Jan 7, 2019

The other issue would be how we handle the errors .... I am still not quite ok with merging the two ... once you have fixed the styling issues I will merge it and decide after I have added some more tests around the authorization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment