Skip to content

[Security] [DataCollector] Remove allows anonymous information in datacollector #41139

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 1 commit into from
May 18, 2021
Merged

[Security] [DataCollector] Remove allows anonymous information in datacollector #41139

merged 1 commit into from
May 18, 2021

Conversation

ismail1432
Copy link
Contributor

@ismail1432 ismail1432 commented May 9, 2021 •
edited by chalasr
Loading

Q A
Branch? 5.2
Bug fix? no
New feature? yes/no
Deprecations? yes/no
Tickets Fix #40907
License MIT
Doc PR symfony/symfony-docs#...

As mentioned In #40907 there is no longer anonymous users no longer in the new authentication system. This PR remove this information if the new system is used as it always a red cross

With enable_authenticator_manager at false
image

With enable_authenticator_manager at true
image

@ismail1432 ismail1432 requested review from chalasr and wouterj as code owners May 9, 2021 13:53
@carsonbot carsonbot changed the title [Security][DataCollector] Remove allows anonymous information in datacollector [Security] [DataCollector] Remove allows anonymous information in datacollector May 9, 2021
@chalasr
Copy link
Member

chalasr commented May 9, 2021

Thanks for the PR.
I think this first has to be a bugfix for 5.2.

As you pointed out, this info is only relevant when the new authenticator system is not in use.
Could you update your patch to remove it only if enable_authenticator_manager is true? And revert the removal of FirewallConfig::allowsAnonymous(), as it will need to be deprecated first when the old system will been removed (in 6.x).

@ismail1432
Copy link
Contributor Author

Thanks @chalasr for the feedback, Yes I'll do 👍

@chalasr chalasr added this to the 5.2 milestone May 9, 2021
@wouterj wouterj mentioned this pull request May 16, 2021
Merged
@ismail1432 ismail1432 force-pushed the security-data-collector-remove-allow-anonymous branch from 4a68e97 to b5f3e27 Compare May 17, 2021 10:47
@ismail1432 ismail1432 force-pushed the security-data-collector-remove-allow-anonymous branch from 9dc4460 to 13b3cc1 Compare May 18, 2021 10:48
@chalasr chalasr changed the base branch from 5.x to 5.2 May 18, 2021 10:57
@chalasr chalasr changed the base branch from 5.2 to 5.x May 18, 2021 10:57
chalasr
chalasr approved these changes May 18, 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.

LGTM for 5.2 (be careful when merging, target branch is incorrect)

@wouterj wouterj changed the base branch from 5.x to 5.2 May 18, 2021 13:25
@ismail1432 ismail1432 changed the base branch from 5.2 to 5.x May 18, 2021 13:25
@wouterj wouterj force-pushed the security-data-collector-remove-allow-anonymous branch from 13b3cc1 to 92cd096 Compare May 18, 2021 13:25
@wouterj wouterj changed the base branch from 5.x to 5.2 May 18, 2021 13:26
@wouterj wouterj merged commit e83c992 into symfony:5.2 May 18, 2021
@wouterj
Copy link
Member

wouterj commented May 18, 2021

Thank you @ismail1432 for fixing the profiler & testing your changes :)

This was referenced May 19, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@wouterj wouterj wouterj approved these changes

@chalasr chalasr chalasr approved these changes

@dunglas dunglas Awaiting requested review from dunglas

@lyrixx lyrixx Awaiting requested review from lyrixx

@xabbuh xabbuh Awaiting requested review from xabbuh

@yceruto yceruto Awaiting requested review from yceruto

Assignees
No one assigned
Labels
Bug Security Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.

[Security][Profiler] “Allows anonymous” always false with enable_authenticator_manager
6 participants
@ismail1432 @chalasr @wouterj @nicolas-grekas @stof @carsonbot