Skip to content

[Security] Display authenticators laziness and exception in the profiler #57382

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

Closed
wants to merge 1 commit into from

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Jun 12, 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 pieces of information to each authenticator displayed in the profiler: its laziness (computed from the return of its supports call) and an eventual AuthenticationException thrown while executing it.

As you can see in the screenshot below (set the profiler’s width to the window’s for the occasion), there may be a better way to display the exception.

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Just a small comment here: what about displaying icons centered? @javiereguiluz

@MatTheCat
Copy link
Contributor Author

MatTheCat commented Jun 18, 2024

I tried displaying authenticators as a list, WDYT?

That way you don’t lose space for missing or redundant data, and you can add some more easily (I have logs in mind e.g.).

@MatTheCat MatTheCat force-pushed the security_profiler branch from 1dfc10a to 7d8c8e1 Compare June 19, 2024 09:54
@MatTheCat
Copy link
Contributor Author

Closing because it diverged from #57369. Will reopen with a redesign and possibly more data.

@MatTheCat MatTheCat closed this Jun 24, 2024
@MatTheCat MatTheCat deleted the security_profiler branch July 9, 2024 15:04
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.

3 participants