Skip to content

[SecurityBundle] Prevent to login/logout without a request context #53172

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
Nyholm merged 1 commit into from
Dec 23, 2023
Merged

[SecurityBundle] Prevent to login/logout without a request context #53172

Nyholm merged 1 commit into from
Dec 23, 2023

Conversation

@symfonyaml
Copy link
Contributor

@symfonyaml symfonyaml commented Dec 21, 2023 •
edited by Nyholm
Loading

Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #53170
License MIT

Using Security::login() in a context without request throws a type error
See all details in the issue #53170

In this PR, we prevent to use Security::login() and Security::logout() without a request context, to avoid a fatal error.

@carsonbot
Copy link

carsonbot commented Dec 21, 2023

Hey!

Oh no, it looks like you have made this PR towards a branch that is not maintained anymore. :/
Could you update the PR base branch to target one of these branches instead? 5.4, 6.3, 6.4, 7.0, 7.1.

Cheers!

Carsonbot

@carsonbot
Copy link

carsonbot commented Dec 21, 2023

Hey!

Thanks for your PR. You are targeting branch "6.2" but it seems your PR description refers to branch "6.2, 6.3, 6.4, 7.0".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@symfonyaml symfonyaml changed the base branch from 6.2 to 6.3 December 21, 2023 10:30
Nyholm
Nyholm reviewed Dec 22, 2023
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

I appreciate this change. Can you fix the CS so it is more similar to the rest of the code? I made a comment on the login() but I want you to do the same thing on the logout().

Could I also ask you to write a test for this?

xabbuh
xabbuh reviewed Dec 22, 2023
View reviewed changes
Nyholm
Nyholm approved these changes Dec 22, 2023
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Im happy with this PR.

The two new tests is written in the same style like other tests in this class.

I aslo think it is a "bugfix" and not a feature because it solves a hard TypeError.

@xabbuh Let us know if we should update the PR and use a real container instead of the mocked one.

xabbuh
xabbuh approved these changes Dec 22, 2023
View reviewed changes
Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

Fine for me then. We can change tests in a separate PR.

@marac19901990
Copy link
Contributor

marac19901990 commented Dec 23, 2023

I ended here somehow. And since here I wanted to be useful so i ran the code and the tests, I hope an extra pair of hands doesn't hurt. It all works and the code is clean. I really prefer the explicit $request === null comparison. Sorry for intruding but since I'm here wanted to support the solution.

@Nyholm
Copy link
Member

Nyholm commented Dec 23, 2023

Thank you. Congratulations to your first Symfony contribution!! And also thank you for all the reviews

@Nyholm Nyholm merged commit 34d44eb into symfony:6.3 Dec 23, 2023
@symfonyaml
Copy link
Contributor Author

symfonyaml commented Dec 23, 2023

It's an honour, thank you very much !

@nicolas-grekas nicolas-grekas added this to the 6.3 milestone Dec 29, 2023
This was referenced Dec 30, 2023
Merged
Merged
Merged
@symfonyaml symfonyaml deleted the patch-1 branch October 21, 2024 14:45
@renovate renovate bot mentioned this pull request Oct 27, 2024
Merged
1 task
@lon9man
Copy link

lon9man commented Nov 8, 2025

Dear maintainers,
I have the case and question.

case:
thinking to migrate from laminas to symfony (due to their deprecation roadmap), so investigating functionality currently.
we have background workers, each worker should process business logic using some concrete virtually logged-in user.

question:
what is the suggested method to login user inside cli-command and then to use it everywhere as it works on web-mode?

thanks!

@OskarStark
Copy link
Contributor

OskarStark commented Nov 9, 2025

Can you please open a new issue ? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nyholm Nyholm Nyholm approved these changes

@xabbuh xabbuh xabbuh approved these changes

@chalasr chalasr chalasr approved these changes

@wouterj wouterj Awaiting requested review from wouterj

Assignees

No one assigned

Labels

Bug SecurityBundle Status: Reviewed

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

9 participants

@symfonyaml @carsonbot @marac19901990 @Nyholm @lon9man @OskarStark @xabbuh @chalasr @nicolas-grekas