Skip to content
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

Improve AuthorizationManager configuration error messages #16194

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

Spikhalskiy
Copy link
Contributor

@Spikhalskiy Spikhalskiy commented Nov 30, 2024

Users that are migrating from Spring 5.x don't know that the AuthorizationManager is turned on by default in 6.x and incompatible with their existing Access Decision Manager configuration.
This improvement to the error message with help to bring their attention to the relevant option that is true by default. It will also prevent them from wasting time and looking at where they implement or wire an AuthorizationManager instance.

Closes #16193

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 30, 2024
@Spikhalskiy
Copy link
Contributor Author

Please feel free to advise if there is a more colloquial format for such additional information in the error messages in Spring framework

@jzheaux
Copy link
Contributor

jzheaux commented Dec 5, 2024

Referring to #16193 (comment), this message is only shown for XML support, so I'm not concerned about there being a conflict. Also, the existing message already uses XML-specific language (use-expressions="true").

I think that an error message should "speak the language" so the remediation is more quickly understood. If the same kind of thing exists in Java Config, that exception message would be different and refer to configuration files, for example.

Unless there is a common situation where a developer would be confused by seeing <http>, I would vote to keep it in the message.

Another possibility would be something like this:

AuthorizationManager and AccessDecisionManager cannot be used at the same time; either remove `access-decision-manager-ref` or add `use-authorization-manager="false"`

I don't think we need to let them know in the error message what the defaults are. We use documentation and IDE support for that. Instead, we should tell them what to do to get up and going again.

Does that address your concern?

@jzheaux jzheaux self-assigned this Dec 5, 2024
@jzheaux jzheaux added in: config An issue in spring-security-config type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 5, 2024
@jzheaux jzheaux added this to the 6.5.x milestone Dec 5, 2024
@Spikhalskiy
Copy link
Contributor Author

This totally makes sense; thank you for the advice. I adjusted the messages, please let me know if they look better.

@jzheaux jzheaux changed the title Improve AuthorizationManager configuration error messages (#16193) Improve AuthorizationManager configuration error messages Dec 5, 2024
@jzheaux jzheaux modified the milestones: 6.5.x, 6.4.2 Dec 5, 2024
Spikhalskiy and others added 2 commits December 5, 2024 14:48
- aligned the grammar
- formatted using gradlew format
- updated copyright year

Issue spring-projectsgh-16193
@jzheaux jzheaux merged commit 3e20f7b into spring-projects:main Dec 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error messaging when use-authorization-manager and access-decision-manager-ref conflict
3 participants