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.

Sign up for GitHub

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

Jump to bottom

[make:auth] Add RememberMeBadge #986

Merged
merged 1 commit into from
Jul 10, 2023
Merged

[make:auth] Add RememberMeBadge #986

merged 1 commit into from
Jul 10, 2023

Conversation

bechir
Copy link
Contributor

@bechir bechir commented Oct 3, 2021 •
edited
Loading

Fix #848

@weaverryan in #848:

EDIT: And I wonder if we should even ask "Do you want to support remember me?" during this process? We could then ask "Do you want remember me to be activated via a checkbox or always activated"? We could use this to determine how the template is generated AND to automatically add the correct remember_me config to security.yaml.

  • Ask "Do you want to support remember me?" and "Do you want remember me to be activated via a checkbox or always activated"?
  • Add RememberMeBadge in LoginFormAuthenticator when remember me is supported
  • Update generated security.yaml
  • Update security/login.html.twig

weaverryan
weaverryan reviewed Oct 12, 2021
View reviewed changes
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Hey @bechir! This is looking awesome! I've dropped some comments. The only other missing piece is a test - check out MakeAuthenticatorTest.php. First, we would need to update any existing tests to answer the new question (with a "no"). Then we need two new test cases that answer (1) yes, yes and (2) yes, no.

src/Maker/MakeAuthenticator.php Outdated
$io->confirm(
'Do you want remember me to be activated via a checkbox or always activated?',
false
)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, the question isn't very clear. Perhaps the user should choose between 2 options instead of a yes/no:

[0] Activate when the user checks a box
[1] Always activate remember me

src/Resources/skeleton/authenticator/Security52LoginFormAuthenticator.tpl.php Outdated
@@ -39,7 +39,7 @@ public function authenticate(Request $request): PassportInterface
new UserBadge($<?= $username_field_var ?>),
new PasswordCredentials($request->request->get('password', '')),
[
new CsrfTokenBadge('authenticate', $request->request->get('_csrf_token')),
new CsrfTokenBadge('authenticate', $request->request->get('_csrf_token')),<?= $remember_me_badge ? "\n\t\t\t\tnew RememberMeBadge(),\n" : "\n" ?>
Copy link
Member

Choose a reason for hiding this comment

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

That's ugly, but I can't remember if there is a nicer way to do it :/

src/Resources/skeleton/authenticator/login_form.tpl.php Outdated Show resolved Hide resolved
src/Resources/skeleton/authenticator/login_form.tpl.php Outdated
@@ -36,6 +59,7 @@
</label>
</div>
#}
Copy link
Member

Choose a reason for hiding this comment

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

Now that we're asking the question explicitly, if the user chooses "no" to remember me, let's not generate anything (so delete the else case and the code above)

src/Security/SecurityConfigUpdater.php
} else {
$firewall['remember_me'][] = $this->manipulator->createCommentLine('always_remember_me: true');
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Wow, you've done an amazing job being VERY user-friendly here :)

@jrushlow jrushlow added the Status: Needs Work Additional work is needed label Feb 22, 2022
@jrushlow jrushlow added the Feature New Feature label Mar 3, 2022
@bechir
Copy link
Contributor Author

bechir commented Oct 4, 2022

Hi @weaverryan !

Thank you for your feedbacks.

Sorry for the late. I just saw that I had a PR in progress.

I updated the code as you suggested me above.

Updated existing tests add added new tests: one for checkbox and an other for always_remember_me (It seems to work)

@bechir bechir requested a review from weaverryan October 4, 2022 06:31
@OskarStark OskarStark changed the title [make:auth] Add RememberMeBadge [make:auth] Add RememberMeBadge Oct 4, 2022
@matthieumota matthieumota mentioned this pull request Mar 23, 2023
Closed
@sadikoff
Copy link
Contributor

Hey @bechir

Sorry for so late ping a lot of time has passed, but will you be able to update your PR and do some last pending fixes?

Cheers!

@bechir
Copy link
Contributor Author

bechir commented Jun 23, 2023

Hi @sadikoff
I rebased my work and tests passed.
Can you check it please?

@weaverryan
Copy link
Member

Sorry for how slow this was to get merged, but thank you for your EXCELLENT work on this feature - very good stuff @bechir!

@weaverryan weaverryan merged commit c283524 into symfony:main Jul 10, 2023
9 checks passed
@weaverryan weaverryan mentioned this pull request Jul 10, 2023
Merged
weaverryan added a commit that referenced this pull request Jul 10, 2023
@weaverryan
minor #1333 Tweaks to rememberme feature (weaverryan)
ee3c156 
This PR was merged into the 1.0-dev branch.

Discussion
----------

Tweaks to rememberme feature

Minor tweaks to #986

Commits
-------

d5bad70 tweaks to rememberme feature
@bechir bechir deleted the add-remember-me-badge branch July 10, 2023 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weaverryan weaverryan Awaiting requested review from weaverryan

Assignees
No one assigned
Labels
Feature New Feature Status: Needs Work Additional work is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make:auth Login form: we should add RememberMeBadge
4 participants
@bechir @sadikoff @weaverryan @jrushlow