logging, added tests, fixed code only flow. #202

brentschmaltz · 2015-03-31T17:26:28Z

still lots of additional testing to come.

renamed id's, spelling _verbose controls test output

HaoK · 2015-03-31T17:35:32Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenidConnectAuthenticationHandler.cs

+
+            if (logLevelDebug)
+            {
+                if (logLevelDebug)


Duplicated debug check?

I will fix that.

HaoK · 2015-03-31T17:44:00Z

yay for tests, only major thing worth considering is removing all of the log level guards for readability, as its really not needed given how straight forward your log message generation is.

brentschmaltz · 2015-04-01T15:13:52Z

This is just the beginning for tests. I was trying to figure out a pattern.

About the annoying external log level check, it's a perf thing, stops the context switch. I plan on lots and lots of logging.

Can you live with it?

Sent from my Windows Phone

From: Hao Kungmailto:notifications@github.com
Sent: ‎3/‎31/‎2015 10:44 AM
To: aspnet/Securitymailto:Security@noreply.github.com
Cc: BrentSchmaltzmailto:brentschmaltz@hotmail.com
Subject: Re: [Security] logging, added tests, fixed code only flow. (#202)

yay for tests, only major thing worth considering is removing all of the log level guards for readability, as its really not needed given how straight forward your log message generation is.

Reply to this email directly or view it on GitHub:
#202 (comment)

kevinchalet · 2015-04-01T21:18:10Z

@brentschmaltz honestly, these changes make the code far harder to read (it's really a PITA 😄)

If you're particularly concerned about performance, why not using the logger extensions that take a format string and multiple arguments instead of manually calling string.Format?

https://github.com/aspnet/Logging/blob/dev/src/Microsoft.Framework.Logging.Interfaces/LoggerExtensions.cs#L52

BTW, @GrabYourPitchforks has done great things with string interpolation in aspnet/DataProtection, and it really makes logging clearer: aspnet/DataProtection@4365b53 😄

GrabYourPitchforks · 2015-04-01T21:59:04Z

@PinpointTownes Thanks for the kind words! Unfortunately one of the downsides to what I did in DataProtection is that logging messages are hardcoded literal strings and thus are non-localized. The code Brent submitted here is more future-proof once localization work is performed.

Calling the logger extensions that take format strings is certainly a good idea. Not only is performance improved, it also gives logging sinks a crack at the raw values without requiring them to parse the string manually.

rynowak · 2015-04-01T22:45:31Z

re: logging - the state of the art is like this: https://github.com/aspnet/Routing/pull/172/files#diff-4997dfe8056272b794ccb3bcdfe214d9R117

Our logging framework understands the key-values - so that loggers that work with key-value-pairs can have them, and loggers that don't still get a new message. /cc @loudej in case he wants to comment.

kevinchalet · 2015-04-01T23:12:09Z

@GrabYourPitchforks interesting, I didn't know that the ASP.NET team wanted to localize tracing messages 😄
Do you know whether tracing components will have their own culture or simply use CultureInfo.CurrentCulture?

I wonder if an i18n + string interpolation mix wouldn't be the best of both worlds: you'd simply call logger.LogWarning($"Request did not match the route '{RouteName}' with template '{RouteTemplate}'.") and it would internally retrieve the localized format string corresponding to "Request did not match the route '{0}' with template '{1}'." and call FormattableString.ToString() if needed.

HaoK · 2015-04-01T23:19:47Z

Actually we decided against localizing log messages at this time since we didn't want admins to potentially know all their users languages. Log Messages should always be in English at this time.

GrabYourPitchforks · 2015-04-02T00:09:55Z

Just to clarify, the existing ASP.NET runtime and other .NET Framework components localize log messages using the system installed culture, not the invoking thread's culture. That way sysadmins always see log messages in their own language. Presumably this will be the behavior in the future, but it's just not ready at this time.

brentschmaltz · 2015-04-02T14:35:26Z

I always use CurrentCulture for the localization reason, just a habit. I actually like to use string.cat as it is much faster than string.format. If you are not convinced, run some numbers on loops of: string.Format vs.
str1 + str2.

About the 'loglevel' check, I favor that even though it's a bit messy, macros remember them made that style neater, I want to minimize the perf hit.

…ts.cs. Added some comments on the 'public' handler and middleware types.

Eilon · 2015-04-02T19:57:27Z

BTW log level guidance is available here: https://github.com/aspnet/Logging/wiki/Guidelines#log-levels

Regarding string concatenation, that doesn't work with resource strings and so should never be used for anything that goes in a resx file (regardless of whether it's ultimately localized).

And regarding the culture, there's no matter of taste or style, it's a correctness issue, per what @GrabYourPitchforks mentioned.

brentschmaltz · 2015-04-06T18:50:06Z

clarifying commnet on what @GrabYourPitchforks said, using String.Format(CultureInfo.CurrentCulture, ...) is correct or NOT correct?

Eilon · 2015-04-06T21:34:36Z

CurrentCulture is incorrect. I think you want InvariantCulture.

brentschmaltz · 2015-04-06T23:16:23Z

Yes, it is not a style thing but a correctness thing. I have always used InvariantCulture when the data was not presented to the user and CurrentCulture when the data was displayed to the user, such as a log entry. Are you saying that is not correct and we should always use InvariantCulture?

Tratcher · 2015-04-06T23:36:06Z

The problem is that there are two users, the remote client and the local admin. CurrentCulture represents the remote client, correct?

brentschmaltz · 2015-04-07T15:54:40Z

Welcome back Tratcher :-)

I thought CurrentCulture represents the culture of the current thread. For logs, I am tailoring them for the dev who uses them for debugging. I know GrabYourPitchforks spoke about the system admin, but I was leaning towards the dev.
If we use InvariantCulture, we are essentially using English culture for all logs when they are localized.

Eilon · 2015-04-07T15:55:54Z

Right, the CurrentCulture will be set to whatever the person using the site/service/API wanted. I.e. someone accessing an API in France could conceivably get French messages. But the developer should always get a single culture. Currently we've said that's the InvariantCulture, but in the future we might want a way for the developer to configure in which culture they want to see all their log messages.

brentschmaltz · 2015-04-07T16:10:13Z

OK, do you want to introduce extensibility for 'culture' now or just go with InvariantCulture.
If we go later, we can always set the default to InvariantCulture.

Eilon · 2015-04-07T16:12:26Z

Right, the dev doing debugging certainly doesn't want to use the culture of the person visiting the site.

I wouldn't do anything special here: just use InvariantCulture.

kevinchalet · 2015-04-07T16:16:02Z

I wouldn't do anything special here: just use InvariantCulture.

Or better, don't use string.Format at all and let aspnet/Logging handle it internally? 😄

brentschmaltz · 2015-04-07T16:21:31Z

@PinpointTownes yep, I am going to investigate just that, let the logger do the string format. Then we just need to modify the logger.

Eilon · 2015-04-07T16:24:20Z

Oh right yeah that's best. Do as little work as possible here, let the logging system do it.

kevinchalet · 2015-04-09T18:14:01Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationOptions.cs

        /// </summary>
        public string SignInScheme
        {
+            // [brentschmaltz] - it is not clear to me why we need both AuthenticationScheme and SignInScheme.
+            // and, why this maps to AuthenticationType.
            get { return TokenValidationParameters.AuthenticationType; }


I fixed SignInScheme in my PR: https://github.com/PinpointTownes/Security/commit/d959eab910a188f9b2cac4b425572fb43decc5ca

I am not ignoring this, but there are some other issues in the runtime that when addressed I am hoping will make this moot.

brentschmaltz · 2015-04-09T18:18:42Z

@Eilon @HaoK @PinpointTownes We have spent a lot of time on style and related items but not bugs, I include a 'codestyle' project in Wilson so that developers can easily confirm to Wilsons style. Can we do the same here?

Eilon · 2015-04-09T20:13:01Z

@brentschmaltz unfortunately StyleCop doesn't support C# 6. We worked on trying to add support for it but it was too complex.

brentschmaltz · 2015-04-10T16:49:26Z

@HaoK @Eilon @PinpointTownes I would like to merge this in, any pushback?

kevinchalet · 2015-04-10T17:16:37Z

@brentschmaltz not particularly. I'll clone and play with the final bits in the next hours 🎮

davidfowl · 2015-04-10T17:45:20Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenidConnectAuthenticationHandler.cs

                Response.StatusCode = 403;
                return;
            }

            if (Response.StatusCode != 401)
            {
+                _logger.LogDebug(Resources.OIDCH_0028_StatusCodeNot401, Response.StatusCode.ToString());


Why are you doing .ToString everywhere?

I do that to ensure a string is passed in the case another API shows up that takes the type.

It will cause the data type to change (obviously), which would affect loggers that store strongly-typed data. E.g. some loggers will store int's as int's, which would allow for richer querying of logs (e.g. select * from logs where log.statusCode >= 400 && log.statusCode < 500).

(As such it's not strictly wrong, but its making the data potentially less useful in some logging systems.)

That is good reasoning, I will change it.

brentschmaltz · 2015-04-10T20:32:25Z

@HaoK good, then who can merge it in?
I am going to explore some additional changes in test. I will post a new PR.

Updated tests Fixed resource string

Conflicts: src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationMiddleware.cs src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationOptions.cs

HaoK · 2015-04-14T17:41:12Z

test/Microsoft.AspNet.Authentication.Test/OpenIdConnect/OpenIdConnectHandlerTests.cs

+        }
+    }
+
+    public class CustomConfigureOptions : ConfigureOptions<OpenIdConnectAuthenticationOptions>


Why do you need a custom configure options that doesn't do anything different than the base?

Good Question. Extensibility.
I want to make sure that my class is called.
Currently, name isn't used, future tests will start making use of the name.

Backed out change Added adjustment for 'ExtenalAuthenticationOptions'

kevinchalet · 2015-04-14T23:23:02Z

src/Microsoft.AspNet.Authentication.OAuth/OAuthAuthenticationOptions.cs

-        /// responsible of persisting user's identity after a successful authentication.
-        /// This value typically corresponds to a cookie middleware registered in the Startup class.
-        /// When omitted, <see cref="ExternalAuthenticationOptions.SignInScheme"/> is used as a fallback value.
+        /// Gets or sets the name of another authentication middleware which will be responsible for actually issuing a user <see cref="System.Security.Claims.ClaimsIdentity"/>.


Huh, why does this PR contain already merged commits? 😢

/cc @HaoK

Rebasing messes up PRs, there are ways to fix it (force push)

Another option is to just create a new PR and branch with squashed changes for final review

I'm definitely not a git expert, but it seems that @brentschmaltz did a git merge instead of a git rebase, which causes these weird issues: https://github.com/brentschmaltz/Security/commit/efaef8952a09c4ec0681db5e0a3c28cb1d96270f

I am also not a git expert, I did a 'git pull' and git went off on its own.
I will tidy it up guys.

@PinpointTownes @HaoK it looks like when I did the pull, it hit a merge conflict. The updated versions are showing as 'unstagged'. I will commit them and push to my fork.

brentschmaltz · 2015-04-15T21:44:50Z

@HaoK Is there anything blocking this pull request?

HaoK · 2015-04-15T21:47:38Z

I don't think there's anything particularly blocking no

brentschmaltz · 2015-04-15T23:26:17Z

@HaoK Can you merge it? I don't think I have the ability to execute the merge.

HaoK · 2015-04-16T09:00:02Z

You've checked in changes before, so you must have permission. Just you will want to squash all your changes together before you push to dev (when you merge this branch, just do git merge --squash), and then it should just be one change that you push to dev

brentschmaltz · 2015-04-16T13:44:21Z

I till try, I had pushed to Katana, but never aspnet.

HaoK · 2015-04-16T19:27:29Z

Merged 501bd4f

brentschmaltz added 2 commits March 30, 2015 11:58

First pass logging and code

f17921c

additional tests for notifications.

4ea3ebe

renamed id's, spelling _verbose controls test output

ghost added the cla-not-required label Mar 31, 2015

HaoK reviewed Mar 31, 2015
View reviewed changes

removed extra loglevel check, renamed file to OpenIdConnectHandlerTes…

e4b4150

…ts.cs. Added some comments on the 'public' handler and middleware types.

Merge branch 'dev' of https://github.com/aspnet/Security into dev

efaef89

kevinchalet reviewed Apr 9, 2015
View reviewed changes

davidfowl reviewed Apr 10, 2015
View reviewed changes

brentschmaltz added 2 commits April 14, 2015 09:52

Added Extension method that takes IOption<T>

a599e14

Updated tests Fixed resource string

Merge branch 'dev' of https://github.com/aspnet/Security into dev

05f2410

Conflicts: src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationMiddleware.cs src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationOptions.cs

HaoK reviewed Apr 14, 2015
View reviewed changes

brentschmaltz added 2 commits April 14, 2015 16:00

Removed ToString()

162f588

Backed out change Added adjustment for 'ExtenalAuthenticationOptions'

Fix name casing.

670437e

kevinchalet reviewed Apr 14, 2015
View reviewed changes

Completing merge

f1fa19b

kevinchalet mentioned this pull request Apr 16, 2015

OpenID connect: support response_mode=query #217

Closed

HaoK closed this Apr 16, 2015

kevinchalet mentioned this pull request Oct 14, 2015

OpenIdConnectOptions API cleanup #478

Closed

logging, added tests, fixed code only flow. #202

logging, added tests, fixed code only flow. #202

Conversation

brentschmaltz commented Mar 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 31, 2015

brentschmaltz commented Apr 1, 2015

kevinchalet commented Apr 1, 2015

GrabYourPitchforks commented Apr 1, 2015

rynowak commented Apr 1, 2015

kevinchalet commented Apr 1, 2015

HaoK commented Apr 1, 2015 via email

GrabYourPitchforks commented Apr 2, 2015

brentschmaltz commented Apr 2, 2015

Eilon commented Apr 2, 2015

brentschmaltz commented Apr 6, 2015

Eilon commented Apr 6, 2015

brentschmaltz commented Apr 6, 2015

Tratcher commented Apr 6, 2015

brentschmaltz commented Apr 7, 2015

Eilon commented Apr 7, 2015

brentschmaltz commented Apr 7, 2015

Eilon commented Apr 7, 2015

kevinchalet commented Apr 7, 2015

brentschmaltz commented Apr 7, 2015

Eilon commented Apr 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentschmaltz commented Apr 9, 2015

Eilon commented Apr 9, 2015

brentschmaltz commented Apr 10, 2015

kevinchalet commented Apr 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentschmaltz commented Apr 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentschmaltz commented Apr 15, 2015

HaoK commented Apr 15, 2015

brentschmaltz commented Apr 15, 2015

HaoK commented Apr 16, 2015

brentschmaltz commented Apr 16, 2015

HaoK commented Apr 16, 2015