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.

Sign up for GitHub

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
+24 −10
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ffdf3be
feat: Add option to exclude ALL auth from check
david-mackessy Dec 18, 2024
75dee35
update javadoc
david-mackessy Jan 6, 2025
877f2a6
Merge branch 'master' into requires-auth-exclude-all
david-mackessy Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions dhis-2/dhis-api/src/main/java/org/hisp/dhis/security/RequiresAuthority.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,11 @@
* Annotation that takes in any number of {@link Authorities}. This allows us to check if a {@link
* org.hisp.dhis.user.User} has any of the {@link Authorities} passed in.
*
* <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.

* Authority allows access to all methods by default. No need to pass {@link Authorities#ALL} in the
* arguments. See {@link AuthorityInterceptor}. <br>
* {@link Authorities#ALL} will only be excluded from the check if explicitly requested, using the
* optional param `excludeAllAuth=true`.
*
* <p>Can be used at Class or Method level. Usage at the method level will always take precedence
* (matching how Spring works). Class level usage only applies if there is no usage at the method
Expand All @@ -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.

}
13 changes: 8 additions & 5 deletions ...dhis-web-api/src/main/java/org/hisp/dhis/webapi/mvc/interceptor/AuthorityInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@
* the passed-in {@link org.hisp.dhis.security.Authorities}. The exception message includes the
* required {@link org.hisp.dhis.security.Authorities} for the endpoint.
*
* <p>{@link Authorities#ALL} is automatically added to the check, as having this Authority allows
* access to all methods by default.
* <p>{@link Authorities#ALL} is only excluded if explicitly requested. The default includes {@link
* Authorities#ALL} in the check, as this is how the system operates.
*/
@Component
public class AuthorityInterceptor implements HandlerInterceptor {
Expand Down Expand Up @@ -88,7 +88,7 @@ public boolean preHandle(
return checkForRequiredAuthority(requiresMethodAuthority);
}

// heck if RequiresAuthority is at method level
// heck if RequiresAuthority is at class level
if (handlerMethod.getBeanType().isAnnotationPresent(RequiresAuthority.class)) {
RequiresAuthority requiresClassAuthority =
handlerMethod.getBeanType().getAnnotation(RequiresAuthority.class);
Expand All @@ -98,8 +98,11 @@ public boolean preHandle(
}

private boolean checkForRequiredAuthority(RequiresAuthority requiresAuthority) {
// include 'ALL' authority in required authorities
List<Authorities> requiredAuthorities = new ArrayList<>(List.of(Authorities.ALL));
List<Authorities> requiredAuthorities = new ArrayList<>();

// include 'ALL' authority in required authorities if not excluded
if (!requiresAuthority.excludeAllAuth()) requiredAuthorities.add(Authorities.ALL);

requiredAuthorities.addAll(List.of(requiresAuthority.anyOf()));

// get user authorities
Expand Down
Loading