alexbeletsky / elmah-mvc

Painless integration of ELMAH into ASP.NET MVC application
http://nuget.org/packages/Elmah.MVC
Apache License 2.0
266 stars 61 forks source link

Make Allowed Users case insensitive. #59

Closed issafram closed 9 years ago

issafram commented 9 years ago

Comparing usernames would be case insensitive after this change. Let me know what you think.

pehadavid commented 9 years ago

i think it should be a web.config settings

issafram commented 9 years ago

@papci I could add that if necessary but is this project still being maintained by @alexbeletsky anymore? Many open pull requests with no action.

alexbeletsky commented 9 years ago

@issafram @papci hey guys, I probably need to make it clear on projects README, but you are right - this project is not actively maintained. The main reason is that I'm not doing .NET development for more than 3 years already.

If someone would like to continue project maintenance, taking care of PR's and submitting patches to Nuget I would happily add you to team.

issafram commented 9 years ago

@alexbeletsky Sorry to see you leave .NET. I don't know who else would like to join the team, but I use ELMAH.MVC in many of my projects, so I would be more than happy to join.

pehadavid commented 9 years ago

hi @issafram & @alexbeletsky,

I'll be glad to continue to maintain this project, as we are using it since many years.

alexbeletsky commented 9 years ago

@issafram @papci that's amazing guys.. I've added you both as collaborators of project, so you can clone this repository and able to directly commit to master and process on PR's

something I don't know how to solve is Nuget package update.. I'll try to relogin to Nuget and maybe collaborators are also there.

issafram commented 9 years ago

@alexbeletsky @papci Try this: https://docs.nuget.org/create/managing-package-owners

alexbeletsky commented 9 years ago

@issafram cool! please send me your's Nuget usernames, so I will update the ownership.

issafram commented 9 years ago

@alexbeletsky Same as my GitHub username. issafram email is the gmail one i've been using.

pehadavid commented 9 years ago

Maybe @issafram could maintain the nuget repository, since I don't have the habbit to deal with nuget admin tools ?

pehadavid commented 9 years ago

and by the way, thanks a lot @alexbeletsky, we will take care of this project :-)

alexbeletsky commented 9 years ago

@issafram just sent you an invitation.

@papci that's actually good idea, let's @issafram keep eye on nuget updates.

once again, I trully appreciate your help. I've released that project for ASP.NET MVC 2.0 and I believe 5.0 is currently actually. let's make our best and provide best support for Elmah on MVC projects.

Now you are having both power and responsibility :) the changes promoted to NuGet have to be carefully tested, so we won't affect a lot of people who are currently using the package. Best luck and let's continue to elaborate here on Github.

alexbeletsky commented 9 years ago

@issafram .. and regarding this issue, I agree to @papci this have to me in configuration, otherwise you would break existing behaviour.

issafram commented 9 years ago

Look at latest commits. I called the setting elmah.mvc.CaseSensitive. Should it be changed or are you guys ok with that?

issafram commented 9 years ago

@papci and @alexbeletsky, if I could just get some input on the setting name, we can then merge the PR. Please look at the above comment. Thank you.

pehadavid commented 9 years ago

Hey @issafram, could you rename it to "UserAuthCaseSensitive" ? i think it's more accurate (because of Role auth)

alexbeletsky commented 9 years ago

I think, logically it's fine, but code I would rewrite in something like that,

var option = Settings.CaseSensitive ? StringComparison. Ordinal : StringComparison. OrdinalIgnoreCase;

return httpContext.Request.IsAuthenticated &&
                   (_allowedUsers.Any(u => u == "*" || u.Equals(httpContext.User.Identity.Name, option)));

to minify usage of if.

issafram commented 9 years ago

@papci @alexbeletsky alright, latest commits are in with changes. And it looks like AppVeyor successfully launched on my push of commits. Are we good to merge?