-
Notifications
You must be signed in to change notification settings - Fork 757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auth provider cleanup #6
base: master
Are you sure you want to change the base?
Conversation
…lResponseParameters population removed on TokenRequest procession
…ference to context property confusing
… to RefreshTokenProvider
…ions per single client/user combination
// TODO: log suspicious activity | ||
return; | ||
} | ||
var user = _repo.FindUserByName(refreshToken.UserName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand this one. You load a refresh token by the hashed token id. Then you load the user by the user name of this token. What is the benefit of this approach? The comment "Refresh token was issued for the other user" doesn't make sense to me. Can you pls explain what your intention was? Thx!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @VILLAN3LL3 ,
Well it's been very long time since I looked in this code so can't guarantee that everything is correct.
From what I see and as far as I remember refresh token hash is the only thing which is required to authorize the user and issue a fresh access token.
So the only way to know for which user to issue a new access token is to keep user name associated to refresh token. The code in line 78 looks up the user in the database and check in the line 79 is redundant if you never delete users. I guess I put it there just to avoid resharper warning further down.
So bottom line the comment below is misleading. If we actually end up in the line 83 it means that the user, for whom the token was issued - now doesn't exist in the database.
Hope that helps
Sorry for a messy pull request, it's my first time :) Will try to break down the major changes I made here:
AuthenticationTicketProvider
and reused in bothSimpleAuthorizationServerProvider
andSimpleRefreshTokenProvider
. This was done because of: a) security reasons (ProtectedTicket
was actually a validaccess_token
that could be reaused to access resource server); b) allowing changes in machineKey without any necessity to update database.SessionId
column added toRefreshToken
entity which is generated per each password grant flow. Previously it was impossible to keep user logged in with refresh token on 2 different machines or 2 different browsers simultaneously, now every time user enters credentials a separate record with uniqueSessionId
would be created inRefreshTokens
table without affecting other sessions.Owin.Cors
middlware. This allows to handleOPTIONS
request correnctly. Previously they would be rejected byOwin.OAuth
middleware.