Idea for custom permission annotations that can work on user model and endpoint parameters

[FroMage]

This got moved over from #39663 to start a discussion

I think permissions such as "canwrite" that do not have access to what they can write on are fantasy permissions that do not exist in most cases. In most cases permissions need to know who the current user is, and what objects they're expected to work on, in addition to what operation they will do on the object.

Here's a more realistic example of how we could define custom permissions that work on real objects, in a way that makes them composable (I've intentionally cut the permission logic into smaller units so that different endpoints can compose them finer-grained):

package model.permissions;

import java.util.List;

import org.jboss.resteasy.reactive.RestForm;
import org.jboss.resteasy.reactive.RestPath;

import io.quarkiverse.renarde.security.ControllerWithUser;
import io.quarkus.arc.Arc;
import io.quarkus.hibernate.orm.panache.PanacheEntity;
import io.vertx.ext.web.RoutingContext;
import jakarta.persistence.Entity;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.OneToMany;
import jakarta.ws.rs.POST;

// The entity representing the current user
@Entity
public class User extends PanacheEntity {
	public boolean isAdmin;
	public String username;
}

// A project belongs to an owner, has a unique name
@Entity
class Project extends PanacheEntity {
	@ManyToOne
	public User owner;
	public String name;
	@OneToMany
	public List<Contributor> contributors;
	@OneToMany
	public List<Team> teams;
	
	public static Project findByName(String projectName) {
		return find("name", projectName).singleResult();
	}
}

enum ContributionType {
	Admin, Read, Write;
}

// A Contributor has specific permissions on a project
@Entity
class Contributor extends PanacheEntity {
	@ManyToOne
	public User owner;
	public ContributionType type;
	@ManyToOne
	public Project project;
}

// A team aggregates people with a single permission on a project
@Entity
class Team extends PanacheEntity {
	@ManyToOne
	public ContributionType type;
	@ManyToOne
	public Project project;
	@OneToMany
	public List<User> members;
}

// The main permission type to extend, single method
interface QuarkusPermission {
	boolean isAllowed();
}

// Allows access to the routing context (simplistic)
interface QuarkusRoutedPermission extends QuarkusPermission {
	default RoutingContext getRoutingContext() {
		return Arc.container().instance(RoutingContext.class).get();
	}
}

// Quarkus user's main permission type, binds permissions to their model of the logged in user
interface MyPermission extends QuarkusRoutedPermission {
	default User getUser() {
		return Arc.container().instance(User.class).get();
	}
}

// Quarkus user's root for permissions around projects: needs a user and project
interface ProjectPermission extends MyPermission {
	default Project getProject() {
		String projectName = getRoutingContext().pathParam("projectName");
		if(projectName == null)
			return null;
		return Project.findByName(projectName);
	}
}

// Current user is admin
interface IsAdminPermission extends MyPermission {
	@Override
	default boolean isAllowed() {
		return getUser().isAdmin;
	}
}

// Current user is owner of project
interface IsProjectOwnerPermission extends ProjectPermission {
	@Override
	default boolean isAllowed() {
		Project p = getProject();
		return p != null && p.owner == getUser();
	}
}

// Current user is project admin of project
interface IsProjectAdminPermission extends ProjectPermission {
	@Override
	default boolean isAllowed() {
		Project p = getProject();
		if(p == null) {
			return false;
		}
		User user = getUser();
		for (Contributor contributor : p.contributors) {
			if(contributor.owner == user && contributor.type == ContributionType.Admin) {
				return true;
			}
		}
		for (Team team : p.teams) {
			if(team.type == ContributionType.Admin) {
				for (User member : team.members) {
					if(member == user) {
						return true;
					}
				}
			}
		}
		return false;
	}
}

// Current user can rename project
interface CanRenameProjectPermission extends IsAdminPermission, IsProjectOwnerPermission, IsProjectAdminPermission {

	@Override
	default boolean isAllowed() {
		return IsAdminPermission.super.isAllowed() || IsProjectOwnerPermission.super.isAllowed() || IsProjectAdminPermission.super.isAllowed();
	}
}

// Annotations for those permissions
@interface CustomPermission{
	Class<? extends QuarkusPermission> value();
}

@CustomPermission(IsAdminPermission.class)
@interface IsAdmin {}

@CustomPermission(CanRenameProjectPermission.class)
@interface CanRenameProject {}

class SecureController extends ControllerWithUser<User> {
	
	// public: no permission required
	Project readProject(@RestPath String projectName) {
		Project project = Project.findByName(projectName);
		notFoundIfNull(project);
		return project;
	}

	@CanRenameProject
	@POST
	void renameProject(@RestPath String projectName, @RestForm String newName) {
		Project project = Project.findByName(projectName);
		// assume we have permission, since it's annotated, also implies the project must already exist, so no need to check for null
		project.name = newName;
		
	}
}

There is the problem of figuring out what to do in case for example the project does not exist, which means the permission can't be granted, so we'd get a 401 instead of a 404 that we would be able to do if we did get into the endpoint to check for this before we checked for permissions.

Now, the bigger problem here is the implementation of ProjectPermission.getProject() because this is the point where it must access endpoint parameters, and then we get into lots of questions related to transactions, security, deserialisation.

The proposed implementation using RoutingContext does not work when using Quarkus REST.

We could add hooks into Quarkus REST to make it easy to access the endpoint parameters (as they would be passed to the endpoint, so after deserialisation) but that would mean invoking the security check after deserialisation and filters, while ATM I suppose it's checked before. But that would be more useful for security checks. Closer to what one could write in code checks.

Well, it's pretty much required anyway, because we'll need a transaction open to do anything useful, and that's done by the CDI interceptor which currently runs after deserialisation (not sure about filters, have to check).

Anyway. Lots of ways to over-engineer security checks if we want annotations. In my experience, doing checks in code is pretty much always required due to the complexity of expressing this in annotations.

Originally posted by @FroMage in #39663 (comment)

[quarkus-bot]

/cc @pedroigor (bearer-token), @sberyozkin (bearer-token,jwt,security)

[sberyozkin]

Thanks @FroMage. Hopefully we'll be able to go ahead with more improvements based on it.

Note though, #39663 is only about making it simpler to apply the existing solution involving @PermissionAllowed referring to custom permission classes with the help of meta-annotations. That still needs to go ahead as planned by @michalvavrik.

But I'm positive we'll be able to support custom security annotations even better with your help, cheers

[sberyozkin]

Hi Steph @FroMage , I think you may be proposing Quarkus own ReBac solution here 👍

[FroMage]

Oh, I didn't know about that acronym. Relationship-based access control. As opposed to role-based. Yeah, I've never worked on projects where roles-based access control was something that would not come out of relations.

[michalvavrik]

If I enhanced @PermissionsAllowed meta-annotation support bit, this @CustomPermission could be such a meta-annotation passed to internal Permission that is also added to the SecurityIdentity. I am mentioning this because enhancing @PermissionsAllowed is easier to maintain then add yet new security annotation. It can be done during the build time and be encapsulated inside Quarkus Security extension.

FWIW I liked idea of @RequiredPermission / @PermissionChecker better, though I suppose we can do both? As long as I am able to make it just built-in Permission it is not really adding anything but some build-time complexity.

[michalvavrik]

FYI I'll have a look into this later this week when #43353 is merged (too many merge conflicts). Let's agree on what should be done and thank you for comments so far.

[sberyozkin]

Just copying an idea from @FroMage here:

If Permission had a boolean isGranted() method, everything would be logical, and we'd be able to write the much more intuitive following code:

// frankly I'd get rid of the Permission supertype here, I don't think it does anything for us
public abstract class QuarkusPermission extends Permission {
 public abstract boolean isGranted(); // perhaps add SecurityIdentity as parameter?
}

// ...
public class BeanParamPermission extends Permission {
    private final String permissionName;
    private final String customAuthorization;
    private final String userName;
    private final String queryParam;
    public BeanParamPermission(String permissionName, String customAuthorizationHeader, String name, String query) {
         super(permissionName);
        this.permissionName = permissionName;
        this.customAuthorization = customAuthorizationHeader;
        this.userName = name;
        this.queryParam = query;
    }
    @Override
    public boolean isGranted() {
        boolean queryParamAllowedForPermissionName = checkQueryParams(queryParam);
        boolean usernameWhitelisted = isUserNameWhitelisted(userName);
        boolean customAuthorizationMatches = checkCustomAuthorization(customAuthorization);
        return queryParamAllowedForPermissionName && usernameWhitelisted
                    && customAuthorizationMatches;
    }
}

Then, you can define a default PermissionChecker (in Quarkus, not needed to be written by the user) that does this for every SecureIdentity:

        var augmentedIdentity = QuarkusSecurityIdentity
                .builder(securityIdentity)
                .addPermissionChecker(requiredPermission -> Uni
                        .createFrom()
                        .item(requiredPermission.isGranted()))
                .build();

Now, if you really insist on keeping Permission, for some reason, I would say it's pretty obvious to me that there are:

• global permissions that a user has, perhaps String permissions, that do not vary based on the request, and you might add QuarkusSecurityIdentity.builder(id).permissions(Permission...) to set them, and you can query them for each request, and use globalPermission.implies(requestedPermission) to figure out if you have a global permission, and if not, then
• per-request permissions that depend on request things, like path, current user, parameters, in which case we use PermissionChecker and see if that permission is granted for this request, and those can't imply other permissions

[sberyozkin]

Hi @michalvavrik and @FroMage, IMHO it indeed makes sense, to do

• QuarkusPermission simplification that you and Steph discussed,
• Let users link to arbitrary bean methods with @PermissionChecker("some-permission")

It will be a next level development for the authorization layer for sure

[michalvavrik]

• QuarkusPermission simplification that you and Steph discussed,
• Let users link to arbitrary bean methods with @PermissionChecker("some-permission")

I'll do that. What shall we do about this issue description:

@interface CustomPermission{
	Class<? extends QuarkusPermission> value();
}

@CustomPermission(CanRenameProjectPermission.class)

I think once we have QuarkusPermission that would extend java.security.Permission, there wouldn't be really difference to @PermissionsAllowed(permission = CanRenameProjectPermission.class) provided we make @PermissionsAllowed#value optional? So now, we could just stick with @PermissionsAllowed and tweak Quarkus Security API?

[michalvavrik]

@FroMage @sberyozkin first thing we need is to agree on API, it doesn't make sense for me to write code before we do. I suggest to continue here: quarkusio/quarkus-security#56