Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

AutoLoginMiddleware #218 #235

Merged
merged 39 commits into from May 6, 2020
Merged

AutoLoginMiddleware #218 #235

merged 39 commits into from May 6, 2020

Conversation

mapeveri
Copy link
Contributor

@mapeveri mapeveri commented Apr 1, 2020

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Tests pass? ✔️

This PR is incomplete, I have some questions:

  1. I understand that the 'remember' cookie is not defined. Where do i have to define it?
  2. For exceptions, what kind of exception do I create?
  3. In getIdentityAndDurationFromCookie at https://github.com/yiisoft/yii2/blob/master/framework/web/User.php#L562 is there a validateAuthKey method. In yii 3 does this method exist?. How can I trigger warnings?.
  4. Is this method in yii 3 exist?.

I think I do not forget anything else, I wait for suggestions and improvements :)

@samdark samdark requested a review from a team April 2, 2020 13:40
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
tests/User/AutoLoginMiddlewareTest.php Show resolved Hide resolved
tests/User/AutoLoginMiddlewareTest.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
@samdark samdark added the status:code review The pull request needs review. label Apr 2, 2020
@samdark samdark requested a review from a team April 2, 2020 22:31
@mapeveri
Copy link
Contributor Author

mapeveri commented Apr 3, 2020

@xepozz @roxblnfk Thank you very much for the feedback. I pay attention to the comments :)

src/User/AutoLoginMiddleware.php Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
tests/User/AutoLoginMiddlewareTest.php Outdated Show resolved Hide resolved
@romkatsu romkatsu requested a review from a team April 4, 2020 11:47
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
tests/User/AutoLoginMiddlewareTest.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
tests/User/AutoLoginMiddlewareTest.php Outdated Show resolved Hide resolved
@romkatsu romkatsu requested a review from a team April 4, 2020 14:57
@xepozz
Copy link
Contributor

xepozz commented Apr 4, 2020

I think we need abstract the credentials storage.
For example, we can store the crentials into 1. cookies, 2. query string, 3. headers, 4. request body, etc.
Later this middleware can be used into yii-rest :)

@mapeveri
Copy link
Contributor Author

mapeveri commented Apr 4, 2020

I updated my code, although I have a doubt with authKey. Whether it is necessary or not.

src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Outdated Show resolved Hide resolved
@samdark
Copy link
Member

samdark commented Apr 5, 2020

As for the questions:

  1. It is usually set on logging in with "remember me" checked in the form. So there should be a service that may do it.
  2. None.
  3. Auth key is required to secure the cookie and allow revoking login status. Warnings could be logged with PSR logger interface.
  4. There's nothing for auth key yet.

@mapeveri
Copy link
Contributor Author

mapeveri commented Apr 6, 2020

@samdark

  1. That is apart from this PR? If not, would you explain a little more.
  2. Ok.
    3 / 4. In this PR, we are going to do something related to this?

@samdark
Copy link
Member

samdark commented Apr 7, 2020

  1. Should be part of the pull request.
    3, 4. Yes or else the pull request won't make much sense.

@mapeveri
Copy link
Contributor Author

mapeveri commented Apr 7, 2020

@samdark ok, great. Can you guide me a little please?.

  1. Where I will add the cookie. In the user login method?
  2. Related to authkey, I check yii2 to see how it does it?. Any suggestion?

Thanks!

@samdark
Copy link
Member

samdark commented Apr 7, 2020

  1. Yes.
  2. Nope. Check Yii 2.

@mapeveri
Copy link
Contributor Author

mapeveri commented Apr 8, 2020

@samdark I updated my PR, but I have some questions.

  1. I added the getAuthKey, validateAuthKey and sendIdentityCookie in User class, but I don't know how save the cookie. I think I need return the response, but not is clear for me. I need a littler help :)

  2. The logout method, I think that need remove the cookie, right?. My doubt is, how to delete the cookie?.

The rest is OK?

Thanks!

@samdark
Copy link
Member

samdark commented Apr 10, 2020

  1. See https://github.com/yiisoft/yii-web/blob/master/src/Cookie.php
  2. Yes, we need to delete cookie. Add one with expiration equals -1.

src/User/User.php Outdated Show resolved Hide resolved
src/User/AutoLogin.php Show resolved Hide resolved
$guestAfterHandle = $this->user->isGuest();

if ($guestBeforeHandle && !$guestAfterHandle) {
$this->autoLogin->addCookie($this->user->getIdentity(false), $response);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not enough tests

изображение

@samdark
Copy link
Member

samdark commented May 6, 2020

TODO:

Create an issue about adding two tests testAddCookieAfterLogin() and testRemoveCookieAfterLogout(). No idea how to implement these in a good way :(

@samdark samdark added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels May 6, 2020
@samdark samdark requested a review from a team May 6, 2020 20:03
@samdark
Copy link
Member

samdark commented May 6, 2020

@mapeveri it's mostly finished except two tests that I have no idea on how to implement in a good way. What do you think?

src/User/Event/AfterLogin.php Show resolved Hide resolved
src/User/Event/BeforeLogin.php Show resolved Hide resolved
src/User/AutoLoginMiddleware.php Show resolved Hide resolved
@samdark samdark merged commit 2f11fc5 into yiisoft:master May 6, 2020
@samdark
Copy link
Member

samdark commented May 6, 2020

Thanks for starting it, @mapeveri.

@mapeveri
Copy link
Contributor Author

mapeveri commented May 7, 2020

@samdark thanks to you. I would like make a some questions, for understand, because I have some doubts. The if of the line https://github.com/mapeveri/yii-web/blob/e89d374cf0b9d2dc838ac449f46e9dde510d963a/src/User/AutoLoginMiddleware.php#L43

and the line https://github.com/mapeveri/yii-web/blob/e89d374cf0b9d2dc838ac449f46e9dde510d963a/src/User/AutoLoginMiddleware.php#L47

I don't understands this. It is not clear to me when each condition is met to add the cookie or not. The login is done on this line: https://github.com/mapeveri/yii-web/blob/e89d374cf0b9d2dc838ac449f46e9dde510d963a/src/User/AutoLoginMiddleware.php#L97

@samdark
Copy link
Member

samdark commented May 7, 2020

These conditions are to detect if user logged in (was guest, became non-guest) or logged out (was non-guest, became guest) in the current request. On login we're setting a cookie. On logout we're removing a cookie. This is totally separated from logging user in by existing cookie that it happening regardless in the very beginning.

@mapeveri
Copy link
Contributor Author

mapeveri commented May 7, 2020

@samdark thanks!!

devanych pushed a commit that referenced this pull request Feb 1, 2021
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
devanych pushed a commit that referenced this pull request Feb 1, 2021
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
devanych pushed a commit that referenced this pull request Feb 1, 2021
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
devanych pushed a commit that referenced this pull request Feb 1, 2021
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
devanych pushed a commit that referenced this pull request Feb 1, 2021
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
devanych pushed a commit that referenced this pull request Feb 1, 2021
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
devanych pushed a commit that referenced this pull request Feb 1, 2021
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:code review The pull request needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants