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

feat: Add option to exclude ALL auth from check #19521

Closed
wants to merge 3 commits into from

Conversation

david-mackessy
Copy link
Contributor

@david-mackessy david-mackessy commented Dec 18, 2024

This change allows us to exclude the ALL authority from an endpoint authority check.
The use for this is probably very low right now, but given it's such a small change and it has no impact on the current behaviour, it seems like a nice option to be able to choose. It's a good topic for conversation at least.

We currently allow the use of ALL to do anything in the system. A recent conversation during a merge demo brought up the topic of the ALL authority. Given that users know about the ALL authority, it is reported that a lot of users are granted the ALL so they can just use the system without hassle and use features they possibly should not be allowed to.

There are no endpoints that use this feature in this change, so no tests can be added yet.

* <p>{@link Authorities#ALL} is automatically added to the check, as having this Authority allows
* access to all methods by default. No need to pass {@link Authorities#ALL} in the arguments. See
* {@link AuthorityInterceptor}.
* <p>{@link Authorities#ALL} is automatically added to the check by default, as having this
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this documentation is too much about how it works and to little about what the goal is on a semantic level and why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the javadocs in this file. Let me know if it's any better/clearer.

@@ -50,4 +52,6 @@
@Retention(RetentionPolicy.RUNTIME)
public @interface RequiresAuthority {
Authorities[] anyOf();

boolean excludeAllAuth() default false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have javadoc explaining what it means to use true. I am actually not really sure even after reading some code. Is this basically deactivating the ALL superuser for being specially privileged and allowed to do anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the javadocs in this file. Let me know if it's any better/clearer.

Copy link

sonarqubecloud bot commented Jan 6, 2025

@david-mackessy
Copy link
Contributor Author

discussed at platform backend team meeting, feedback was that we might tackle the ALL auth in the future (how it's used system-wide), so best not to introduce divergences in how that works for now.

@david-mackessy david-mackessy deleted the requires-auth-exclude-all branch January 8, 2025 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants