Skip to content

[Security] Add authenticators info to the profiler #42582

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
merged 1 commit into from
Oct 11, 2021

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Aug 15, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

The profiler's security panel needs an update regarding the not-so-new authenticator manager system.
Here it is:
Screenshot 2021-10-10 at 21 09 40

@chalasr chalasr requested a review from wouterj as a code owner August 15, 2021 18:03
@carsonbot carsonbot changed the title [Security][WIP] Add authenticators info to the profiler [Security] [WIP] Add authenticators info to the profiler Aug 15, 2021
@wouterj
Copy link
Member

wouterj commented Aug 15, 2021

Nice! This would fix #36668 (maybe there are some ideas in there that you didn't think of).

On a related note: I think it would make sense to start using tabs (like on the request page), instead of putting everything (voting, authenticators, firewall config, etc) on a single page.

@wouterj
Copy link
Member

wouterj commented Aug 15, 2021

And a DX thing that we probably should not try to fix in this PR: the information of this profiler will mostly be useful for the routes doing authentication. We should maybe introduce a way to link to profilers of previous requests from the toolbar (maybe even show some info), as e.g. you'll never get to see the profiler data of the actual login requests in the login form (as you're always redirected - either back to the form or to the target page).

</braindump about profiler improvements for new security> 😉

@chalasr
Copy link
Member Author

chalasr commented Aug 15, 2021

I totally forgot about that issue, thank you.
About the need for improving this screen, I agree. I’ll take some inspiration from other panels. I’d welcome any suggestion also (/cc @javiereguiluz).

We should maybe introduce a way to link to profilers of previous requests from the toolbar (maybe even show some info), as e.g. you'll never get to see the profiler data of the actual login requests in the login form (as you're always redirected - either back to the form or to the target page).

Definitely, I’ll create an issue for that.

@nicolas-grekas nicolas-grekas added this to the 5.4 milestone Aug 18, 2021
@chalasr chalasr force-pushed the authenticator-profiler branch from 769aa64 to d0ca257 Compare September 27, 2021 10:54
@chalasr chalasr force-pushed the authenticator-profiler branch 8 times, most recently from fc17f10 to 3095273 Compare October 10, 2021 15:01
@chalasr chalasr changed the title [Security] [WIP] Add authenticators info to the profiler [Security] Add authenticators info to the profiler Oct 10, 2021
@chalasr chalasr force-pushed the authenticator-profiler branch 5 times, most recently from f9fbdf4 to 8789469 Compare October 10, 2021 16:13
@chalasr chalasr force-pushed the authenticator-profiler branch from 8789469 to 3ec5e96 Compare October 10, 2021 16:41
@chalasr
Copy link
Member Author

chalasr commented Oct 10, 2021

Now with tests. Also the security panel content is now split in several tabs, as suggested by Wouter.
(diff best viewed without whitespaces https://github.com/symfony/symfony/pull/42582/files?w=1)

PR ready!

@fabpot
Copy link
Member

fabpot commented Oct 11, 2021

Thank you @chalasr.

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

Successfully merging this pull request may close these issues.

5 participants