Skip to content

[Security] Add badge resolution to profiler #51585

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.

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

Merged

Conversation

Jean-Beru
Copy link
Contributor

@Jean-Beru Jean-Beru commented Sep 6, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets #36668
License MIT
Doc PR

This PR add badges resolution status in Security profiler as mentioned in #36668 (" See which badges are resolved and which aren't").

CSRF error

image

Wrong credentials

image

Authentication successful

image

@Jean-Beru Jean-Beru requested a review from chalasr as a code owner September 6, 2023 16:42
@carsonbot carsonbot added Status: Needs Review DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Security labels Sep 6, 2023
@carsonbot carsonbot added this to the 6.4 milestone Sep 6, 2023
@carsonbot carsonbot changed the title [DX][Security] Add badge resolution to profiler [Security] Add badge resolution to profiler Sep 6, 2023
@fabpot
Copy link
Member

fabpot commented Sep 10, 2023

Thank you @Jean-Beru.

@fabpot fabpot force-pushed the feature/add-badge-resolution-to-profiler branch from 3a810c1 to 2324da2 Compare September 10, 2023 09:07
@fabpot fabpot merged commit fee9b30 into symfony:6.4 Sep 10, 2023
@PhilETaylor
Copy link
Contributor

Composer updated a stable project and now broken :(

ScreenShot-2023-09-10-15 26 32

@PhilETaylor
Copy link
Contributor

ok on debugging I can see that on the login request, when viewed In debugger, I can see the badges - so the point of this PR is working fine.

But after login when redirected to the user dashboard of the app, and trying to load any page when already logged in, then the passport is null in the TraceableAuthenticator and this means the whole app is down with the error previously provided.

Changing $this->passport->getBadges() to $this->passport?->getBadges() ?? [] suppresses the error

ScreenShot-2023-09-10-15 48 36

@chalasr
Copy link
Member

chalasr commented Sep 10, 2023

Would you mind sending a PR (with test ideally)?

@PhilETaylor
Copy link
Contributor

like this #51612 ?

fabpot added a commit that referenced this pull request Sep 10, 2023
…th (PhilETaylor)

This PR was merged into the 6.4 branch.

Discussion
----------

[Security] Fix for TraceableAuthenticator debug when no Auth

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #51585 (comment)
| License       | MIT

Fixes regression in PR #51585 and adds unit test

![image](https://github.com/symfony/symfony/assets/400092/8645c3c4-1827-4dce-83b6-a1cdace8dfb1)

Commits
-------

a9d36a5 Fix for TraceableAuthenticator debug when no Auth
This was referenced Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Security Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants