Repository navigation
State and tests - #301
State and tests#301brentschmaltz wants to merge 7 commits into
Conversation
Removed AuthenticationProperties as a Property on notification. Added tests for sending and receiving 'user' state.
There was a problem hiding this comment.
Fix this comment.
|
Close your old PR if you're done with it. |
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) |
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. |
There was a problem hiding this comment.
yes but there is no overload that has a message.
Replacement #233
Changed logic for user-state
Added additional testing