Skip to content

[SecurityBundle] Improve profiler’s authenticators tab #57525

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
Jul 25, 2024

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Jun 25, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix parts of #36668
License MIT

This PR adds two new pieces of data to the profiler’s security panel’s authenticators tab: their “laziness” (if their supports method returned null) and the exception passed to their onAuthenticationFailure method.

It also redesigns the table because displaying every possible column leads to a lot of wasted space and decreases legibility:


(You can see the table overflowing its container and the screen.)

Instead, I took inspiration from the messenger panel and

  • reduced the number of columns to two: “status” (skipped/success/failure) and authenticator’s data
  • put additional data behind a toggle, expanded by default for authenticators whose authenticate method was called
  • wrote yes/no instead of using icons to get rid of the sense of rightness/wrongness

This will also make easier to add data if needed.

@chalasr
Copy link
Member

chalasr commented Jun 27, 2024

The duration and laziness info feel less readable/structured to me. Looks good apart from that

@MatTheCat MatTheCat changed the title [SecurityBundle] Improve authenticators tab [SecurityBundle] Improve profiler’s authenticators tab Jun 28, 2024
@MatTheCat MatTheCat closed this Jul 9, 2024
@MatTheCat MatTheCat deleted the new_authenticator_profiler branch July 9, 2024 15:02
@MatTheCat MatTheCat restored the new_authenticator_profiler branch July 9, 2024 15:03
@MatTheCat MatTheCat reopened this Jul 9, 2024
@MatTheCat
Copy link
Contributor Author

MatTheCat commented Jul 12, 2024

Okay tried to iterate on my previous idea (code not pushed yet):

Does that look better?

@MatTheCat MatTheCat force-pushed the new_authenticator_profiler branch from 80b1ae1 to 77872ba Compare July 13, 2024 15:15
@MatTheCat
Copy link
Contributor Author

Pushed previously mentioned iteration with small modifications. Updated the PR description accordingly.

@MatTheCat MatTheCat force-pushed the new_authenticator_profiler branch from 77872ba to 771450c Compare July 13, 2024 15:38
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.

Thanks, I like it. Maybe the lazy info could be a flag instead of yes/no as done elsewhere. Pinging @javiereguiluz in case you can help making this even better.

@MatTheCat
Copy link
Contributor Author

MatTheCat commented Jul 13, 2024

Yay 😀

By “flag”, do you mean “icon”?
I replaced these by text because I felt the sense of good/bad they carry did not fit with authenticators’ data (e.g. it is not “good” if one is lazy, as it is not “bad” if it isn’t).

@chalasr
Copy link
Member

chalasr commented Jul 13, 2024

Makes sense 👍

@MatTheCat MatTheCat force-pushed the new_authenticator_profiler branch 2 times, most recently from 40ab8d0 to 619d8e4 Compare July 15, 2024 12:32
@MatTheCat MatTheCat force-pushed the new_authenticator_profiler branch from 619d8e4 to a8075d4 Compare July 17, 2024 11:36
@fabpot
Copy link
Member

fabpot commented Jul 25, 2024

Thank you @MatTheCat.

@fabpot fabpot merged commit 0ca5232 into symfony:7.2 Jul 25, 2024
10 checks passed
@MatTheCat MatTheCat deleted the new_authenticator_profiler branch July 25, 2024 07:04
@fabpot fabpot mentioned this pull request Oct 27, 2024
nicolas-grekas added a commit that referenced this pull request Jan 6, 2025
…oggles (MatTheCat)

This PR was merged into the 6.4 branch.

Discussion
----------

[WebProfilerBundle] Fix event delegation on links inside toggles

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | N/A
| License       | MIT

In #57525, the security panel’s authenticator tab got updated with dumps in toggles. Issue is, we currently call `stopPropagation` when link are clicked to avoid closing the toggle, but dumps handle links click using event delegation. That means when you click on a `sf-dump-toggle`, the event cannot reach the `sf-dump` because its propagation is stopped.

This PR handles this case by checking if a link is present in the DOM between the toggle and the element which was clicked.

It also leverages the `currentTarget` property to get the clicked toggle.

Commits
-------

241597d [WebProfilerBundle] Fix event delegation on links inside toggles
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.

4 participants