Skip to content
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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

2ooom
Copy link

@2ooom 2ooom commented Oct 28, 2014

Sorry for a messy pull request, it's my first time :) Will try to break down the major changes I made here:

  1. Access Tokens are not persisted into database now. Instead tickets issuing logic is factored out to AuthenticationTicketProvider and reused in both SimpleAuthorizationServerProvider and SimpleRefreshTokenProvider. This was done because of: a) security reasons (ProtectedTicket was actually a valid access_token that could be reaused to access resource server); b) allowing changes in machineKey without any necessity to update database.
  2. Allowing multiple sessions for single user on different browsers or machines. SessionId column added to RefreshToken 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 unique SessionId would be created in RefreshTokens table without affecting other sessions.
  3. Allow Origin logic factored out to Owin.Cors middlware. This allows to handle OPTIONS request correnctly. Previously they would be rejected by Owin.OAuth middleware.

…lResponseParameters population removed on TokenRequest procession
// TODO: log suspicious activity
return;
}
var user = _repo.FindUserByName(refreshToken.UserName);

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!

Copy link
Author

@2ooom 2ooom May 21, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants