Skip to content

[Security] Add a method in the security helper to ease programmatic login (#40662) #41274

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
Merged
merged 2 commits into from
Jul 5, 2022
Merged

[Security] Add a method in the security helper to ease programmatic login (#40662) #41274

merged 2 commits into from
Jul 5, 2022

Conversation

johnkrovitch
Copy link
Contributor

@johnkrovitch johnkrovitch commented May 19, 2021 •
edited
Loading

Q A
Branch? 6.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #40662
License MIT
Doc PR

This PR aims to ease the programmatic login using the Security helper, to fix (#40662).

A simple method has been added to the Security helper. It take a user and an optional Authenticator. If no authenticator is passed, we find all authenticators for the current firewall. Then if only one is matching we use this one. If several authenticators are found, an exception is thrown to avoid any magic (by choosing the first for example), the user HAS to provide an authenticator.

Thanks

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@chalasr chalasr added this to the 5.x milestone May 19, 2021
@carsonbot
Copy link

Hey!

I think @fancyweb has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@emancypage
Copy link

emancypage commented Jun 2, 2021 •
edited
Loading

Hello @johnkrovitch!
I tried to use your code in my environment and I noticed one issue here. In case of this configuration for firewall security:

firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false

I've got an error message:
image

You are assuming that we always will have fourth parameter from \Symfony\Bundle\SecurityBundle\DependencyInjection\SecurityExtension::createFirewall() but please look into this line of code:

symfony/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

Line 365 in 2fe4442

return [$matcher, [], null, null];

This also should be adjusted in your code I think.

@johnkrovitch johnkrovitch force-pushed the feature/auto-login branch 2 times, most recently from 9109437 to aabf498 Compare June 3, 2021 12:40
@johnkrovitch
Copy link
Contributor Author

Thanks for your response. It is fixed.

chrishalbert
chrishalbert reviewed Aug 5, 2021
View reviewed changes
Copy link

@chrishalbert chrishalbert left a comment

Choose a reason for hiding this comment

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

I have a few suggestions regarding backwards compatibility and semantic versioning.

@chalasr chalasr mentioned this pull request Aug 7, 2021
Closed
@johnkrovitch johnkrovitch force-pushed the feature/auto-login branch 4 times, most recently from b02ef09 to 1071c90 Compare August 8, 2021 15:50
@fabpot fabpot modified the milestones: 5.4, 6.1 Nov 16, 2021
chalasr
chalasr requested changes Dec 3, 2021
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Some comments. A CHANGELOG entry will be needed also

@johnkrovitch johnkrovitch force-pushed the feature/auto-login branch 2 times, most recently from c2878ab to 87b0044 Compare December 6, 2021 20:46
@johnkrovitch
Copy link
Contributor Author

The security change log has been updated

@chalasr chalasr force-pushed the feature/auto-login branch 3 times, most recently from c510d9b to 5e756a6 Compare July 4, 2022 16:21
@chalasr
Copy link
Member

chalasr commented Jul 4, 2022

Now with a functional test. Remaining comments addressed. PR ready

@chalasr chalasr force-pushed the feature/auto-login branch 5 times, most recently from 949aa71 to f31bf35 Compare July 5, 2022 12:56
@chalasr chalasr force-pushed the feature/auto-login branch 3 times, most recently from 2ff2369 to 78eaf86 Compare July 5, 2022 20:21
@chalasr chalasr force-pushed the feature/auto-login branch from 78eaf86 to 37efa72 Compare July 5, 2022 20:29
@chalasr
Copy link
Member

chalasr commented Jul 5, 2022

It took time, but here we go, this is in now. Thank you very much @johnkrovitch.

@chalasr chalasr mentioned this pull request Jul 5, 2022
Closed
@chalasr chalasr merged commit 6243772 into symfony:6.2 Jul 5, 2022
@johnkrovitch johnkrovitch deleted the feature/auto-login branch October 5, 2022 09:01
@fabpot fabpot mentioned this pull request Oct 24, 2022
Merged
@hellomedia
Copy link
Contributor

hellomedia commented Dec 1, 2022 •
edited
Loading

Thanks for this.

The authenticate method can return a redirect response - when redirection logic is defined in onAuthenticationSuccess.

Shouldn't the login method return the same - by returning the return value of authenticate ?

symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jul 28, 2023
@fabpot
feature #46094 [Security][SecurityBundle] Move the Security helper …
c3dfdd2 
…to SecurityBundle (chalasr)

This PR was merged into the 6.2 branch.

Discussion
----------

[Security][SecurityBundle] Move the `Security` helper to SecurityBundle

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | Fixes symfony/symfony#46066 (comment)
| License       | MIT
| Doc PR        | todo

The `Security` helper is a high-level service providing an easy access to commonly-needed features coming from various low-level abstractions. Basically, it's a facade.
Based on this, it makes sense to me to make it available only via the full-stack framework, as proposed by Wouter in symfony/symfony#46066 (comment).

This unlocks #46066, symfony/symfony#41274 and symfony/symfony#41406.
/cc @wouterj @johnkrovitch @Kocal

Commits
-------

7b91bcb068 [Security] Move the `Security` helper to SecurityBundle
@94noni 94noni mentioned this pull request Mar 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weaverryan weaverryan weaverryan left review comments

@1ed 1ed 1ed left review comments

@chalasr chalasr chalasr approved these changes

@chrishalbert chrishalbert chrishalbert left review comments

@fabpot fabpot fabpot approved these changes

@kbond kbond kbond approved these changes

@wouterj wouterj Awaiting requested review from wouterj wouterj is a code owner

Assignees
No one assigned
Labels
Feature Ready Security Status: Reviewed
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.

[Security][DX] RFC: A simple way to do programmatic login