Security - Ability to create custom permission annotation

[zZHorizonZz]

Description

Inspired by a conversation on Zulip: https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/.E2.9C.94.20grpc.20-.20current.20grpc.20message.20in.20auth, I propose a new feature for custom permission annotation for gRPC and REST.

Concept:

Allow creation of custom permission annotations, themselves annotated with @PermissionsAllowed.
You would be able to define default permission values within these annotations and other values as like for example value itself.
Optionally, it would be nice to have passing custom parameters to the custom permission class from custom permission (Explained in implementation), if specified.

Implementation ideas

As suggested in conversation you would be able to do this:

@PermissionsAllowed(permission = CustomPermission.class, value = "request")
public @interface CustomPermissionAnnotation {

}

Or you would be able to do this:

@PermissionsAllowed(permission = CustomPermission.class, value = "request")
public @interface CustomPermissionAnnotation {
    CustomEnumType customEnumType() default CustomEnumType.READ;
}

public class CustomPermission extends Permission {

    private final String name;
    private final String[] actions;
    private final CustomEnumType customType;

    public CustomPermission(String name, String[] actions, CustomEnumType customEnumType) {
    
    }

You would be then able to do this:

@CustomPermissionAnnotation(customEnumType = CustomEnumType.WRITE)
public String writeCustomPermission(String write) {
    
}

[quarkus-bot]

You added a link to a Zulip discussion, please make sure the description of the issue is comprehensive and doesn't require accessing Zulip

_{This message is automatically generated by a bot.}

[quarkus-bot]

/cc @sberyozkin (security)

[sberyozkin]

CC @michalvavrik

[michalvavrik]

I agree. I'm not sure what stops us from doing that, but I'll add this issue on my list.

[michalvavrik]

I think this is similar to @CanWrite also mentioned here #42957 (comment) so it seems natural people would reduce repetition. We should do this.

[FroMage]

This looks like a good idea. Although, I still 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.

[michalvavrik]

@FroMage man, you really changed the game :-) I believe that what @zZHorizonZz and @jasoncsmith7 described is simply support for meta annotation with @PermissionsAllowed to cut down verbosity. And I was just implementing it as I read your comment :-)

However I don't want to ignore your comment as you put a lot of work into it and there are parts I do agree, not sure what to do, don't want to discard what I am doing as at least 2 users asked for it and meta-annotations support forPermissionsAllowed looks like good easy-to-implement feature.

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.

In custom @PermissionsAllowed permission you can do Arc.container().instance(SecurityIdentity.class) (hmm, I wonder if lazy auth is in place if there can be blocking issue on IO thread, that will depend on endpoint return type) and it does work because CDI request context is activated and identity association is already set.

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

In custom @PermissionsAllowed permission you can do Arc.container().instance(RoutingContext.class) and it does work because CDI request context is activated and CurrentVertxRequest is preset. I have test for it:

quarkus/extensions/resteasy-reactive/rest/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/CustomPermission.java

Line 16 in 155093d

var event = Arc.container().instance(RoutingContext.class).get();

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.

All security checks except for @PermissionsAllowed when method arguments are required run eagerly. For @PermissionsAllowed requiring method arguments, we require authentication (because anonymous identity cannot have permissions) which reduces DoS potential. Then serizalition is run. Therefore you can inject any endpoint method parameters like query param, path param, payload body, whatever.

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).

I don't have test for this, mostly because it would require started database.

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):

I think we need separate issue for your proposal, unless I completely misunderstood issue description. There is GitHub feature "Reference in new issue", it takes just few second, would you mind creating one from your comment above, please? Unless you disagree that meta-annotation support should be implemented. Thanks

[FroMage]

@FroMage man, you really changed the game :-) I believe that what @zZHorizonZz and @jasoncsmith7 described is simply support for meta annotation with @PermissionsAllowed to cut down verbosity. And I was just implementing it as I read your comment :-)

However I don't want to ignore your comment as you put a lot of work into it and there are parts I do agree, not sure what to do, don't want to discard what I am doing as at least 2 users asked for it and meta-annotations support forPermissionsAllowed looks like good easy-to-implement feature.

Yeah, this original issue is probably fine. I'm very skeptical that real applications can get by with just this, though. In my experience, string roles without being able to reason about the target are never useful. But people ask for this, for some reason, so there's probably value.

In custom @PermissionsAllowed permission you can do Arc.container().instance(SecurityIdentity.class) (hmm, I wonder if lazy auth is in place if there can be blocking issue on IO thread, that will depend on endpoint return type) and it does work because CDI request context is activated and identity association is already set.

SecurityIdentity is never useful to any application, besides holding the unique ID of the logged in user. In most applications, the user's model needs to be loaded from the DB from this ID, requiring a TX. That's the only thing that makes sense when dealing with the target of the action.

In custom @PermissionsAllowed permission you can do Arc.container().instance(RoutingContext.class) and it does work because CDI request context is activated and CurrentVertxRequest is preset. I have test for it:

It works, for sure, but it doesn't contain path parameters that have been defined by Quarkus REST. It would only contain those it knows about when defined as Vert.x routes, which is not the case in most applications. So it's not useful for Quarkus REST apps.

All security checks except for @PermissionsAllowed when method arguments are required run eagerly. For @PermissionsAllowed requiring method arguments, we require authentication (because anonymous identity cannot have permissions) which reduces DoS potential. Then serizalition is run. Therefore you can inject any endpoint method parameters like query param, path param, payload body, whatever.

Oh, I did not realise that. This is great. How do you access them, then?

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).

I don't have test for this, mostly because it would require started database.

Yeah, but that's pretty crucial to know.

I think we need separate issue for your proposal, unless I completely misunderstood issue description. There is GitHub feature "Reference in new issue", it takes just few second, would you mind creating one from your comment above, please? Unless you disagree that meta-annotation support should be implemented. Thanks

It could be moved to a new issue, I'm not saying we should do this, it's just a first draft of a more realistic permissions framework that actually can compete with real-life security checks in code. Mostly done to show you how security checks in code, using endpoint parameters and the real DB model would look like if we tried real hard to put it in annotations, and to illustrate that string permissions can't possibly be enough in applications where security requires the DB model.

[FroMage]

Moved to #43238

[michalvavrik]

Just shortly as I am in hurry today:

It works, for sure, but it doesn't contain path parameters that have been defined by Quarkus REST. It would only contain those it knows about when defined as Vert.x routes, which is not the case in most applications. So it's not useful for Quarkus REST apps.
Oh, I did not realise that. This is great. How do you access them, then?

For example, here is test for Quarkus REST:

quarkus/extensions/resteasy-reactive/rest/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/PermissionsAllowedResource.java

Line 52 in f8f7f00

@PermissionsAllowed(value = "farewell", permission = CustomPermissionWithExtraArgs.class, params = { "goodbye", "toWhom",

It accepts @RestPath, @RestHeader and @RestCookie and String which is payload (it could be DTO as well, no difference for impl.). These properties are passed to a permission constructor:

quarkus/extensions/resteasy-reactive/rest/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/CustomPermissionWithExtraArgs.java

Line 14 in f8f7f00

public CustomPermissionWithExtraArgs(String permName, String goodbye, String toWhom, int day, String place) {

You can pass there any endpoint method parameter. Today I used Jakarta REST annotations like PathParam("goodbye") String goodbye, @HeaderParam("toWhom") String toWhom, @CookieParam("day") int day when writing test for a meta-annotation.

It could be moved to a new issue, I'm not saying we should do this, it's just a first draft of a more realistic permissions framework that actually can compete with real-life security checks in code. Mostly done to show you how security checks in code, using endpoint parameters and the real DB model would look like if we tried real hard to put it in annotations, and to illustrate that string permissions can't possibly be enough in applications where security requires the DB model.

Got it. thank you. I agree this @PermissionsAllowed construct could be simplified, let's discuss it there when there is a time.