State and tests #301
State and tests #301
Conversation
Removed AuthenticationProperties as a Property on notification. Added tests for sending and receiving 'user' state.
| } | ||
|
|
||
| // devs will need to hook AuthenticationFailedNotification to avoid having 'raw' runtime errors displayed to users. | ||
| // if any of the error fields are set, return null |
There was a problem hiding this comment.
Fix this comment.
|
Close your old PR if you're done with it. |
| @@ -23,6 +23,9 @@ public class OpenIdConnectAuthenticationHandler : AuthenticationHandler<OpenIdCo | |||
| { | |||
| private const string NonceProperty = "N"; | |||
| private const string UriSchemeDelimiter = "://"; | |||
| // should we consider adding a first class property on AuthenticationProperties similar to RedirectUri. | |||
| // if we don't, then we may want to think about where to make this value public. | |||
| private const string UserState = "userstate"; | |||
There was a problem hiding this comment.
You could put it in OpenIdConnectAuthenticationDefaults.
There was a problem hiding this comment.
I think that would work, will make that change.
Logging level, IsNullOrEmpty for string, comment tests, const for userstate property. SetUserState method has single focus.
…for signin and signout.
|
@brentschmaltz I'm not sure it's really worth it. Anyway, we'll probably switch to good old methods in a future PR to match the pattern used by the OAuth2 middleware, so you won't have to care about null values in the handler itself: #257 (comment) |
| @@ -586,34 +621,13 @@ private string ReadNonceCookie(string nonce) | |||
| return null; | |||
| } | |||
|
|
|||
| private AuthenticationProperties GetPropertiesFromState(string state) | |||
| private AuthenticationProperties SetUserStateOnMessage(OpenIdConnectMessage message, AuthenticationProperties properties) | |||
There was a problem hiding this comment.
This method seems rather trivial and is used only once in the handler. Maybe you can remove it?
There was a problem hiding this comment.
@PinpointTownes can you help me understand about leaving the notifications NON null (setting default no-ops in OpenIdConnectAuthenticationNotifications). I thinks there is a gain here, if the user hasn't set a notification, then no need to new up the Notification and call a no-op.
I opened
#307
+1 on removing the mini-method: SetUserStateOnMessage
|
@Eilon this PR has been marked cla-required. What do I need to do? |
|
@brentschmaltz I sent you mail about the CLA. |
| await server.CreateClient().PostAsync("http://localhost", new FormUrlEncodedContent(message.Parameters)); | ||
| LoggingUtilities.CheckLogs(loggerFactory.Logger.Logs, expectedLogs, errors); | ||
| Debug.WriteLine(LoggingUtilities.LoggingErrors(errors)); | ||
| Assert.True(errors.Count == 0, LoggingUtilities.LoggingErrors(errors)); |
There was a problem hiding this comment.
yes but there is no overload that has a message.
| /// <summary> | ||
| /// Used to set up different configurations of metadata for different tests | ||
| /// </summary> | ||
| public class ConfigurationManager |
|
@brentschmaltz, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
Some types to vars. urls -> example.com
…to StateAndTests Conflicts: test/Microsoft.AspNet.Authentication.Test/OpenIdConnect/OpenIdConnectHandlerTests.cs test/Microsoft.AspNet.Authentication.Test/OpenIdConnect/OpenIdConnectMiddlewareTests.cs test/Microsoft.AspNet.Authentication.Test/OpenIdConnect/TestUtilities.cs
|
Making progress. You'll need to rebase after all the changes that went into Security last week. |
|
@Tratcher there are too many diff's to rebase.I will have to craft a new PR. |
|
@brentschmaltz why are you so afraid of rebasing? 😄 |
|
updated |
Replacement #233
Changed logic for user-state
Added additional testing