OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.45k stars 2.4k forks source link

Remove unnecessary null check in 2FA authorization filter #17009

Open hishamco opened 1 week ago

MikeAlhayek commented 1 week ago

https://github.com/search?q=repo%3AOrchardCMS%2FOrchardCore%20.HttpContext%3F.User&type=code

hishamco commented 1 week ago

I didn't realize there were many places :) I just notice it in that place. I will fix it in all the places

MikeAlhayek commented 1 week ago

I don't think you should remove the null check. If the request come from a non HTTP request like background task, there won't be user.

hishamco commented 1 week ago

If the request come from a non HTTP request like background task, there won't be user.

We might need a unit test for that

MikeAlhayek commented 1 week ago

We might need a unit test for that

I would not make such a global change blindly. I would address them case by case if needed. It's probably safer to leave everything as is and close the PR :)

hishamco commented 1 week ago

If that's the case I presume to see all .NET Core libraries using nullable users. I will make a unit or integration test to make sure that the user could not be null

hishamco commented 1 week ago

If I'm not wrong one of the use cases that we have is HTTP Workflow

MikeAlhayek commented 19 hours ago

@hishamco if you like me think this PR is not useful, please close it.

hishamco commented 19 hours ago

The question why did you add the nullable operator :)

MikeAlhayek commented 19 hours ago

as previously explained, HttpContext could be null if the request is not HTTP like from a background task.

hishamco commented 19 hours ago

If you are right the User will be null all the time, let me create a unit or functional test for that instead of a long discussion :)

gvkries commented 7 hours ago

As per the nullable annotations, IHttpContextAccess.HttpContext? can be null, but HttpContext.User is never null.