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

[breaking] Variabilized security roles #223

Merged
merged 11 commits into from May 11, 2016

Conversation

Projects
None yet
2 participants
@fcamblor
Contributor

fcamblor commented Oct 27, 2015

This PR allows to define "variabilized" security roles based on what we have in query params or path param for a dedicated endpoint.

It will allow to typically have such endpoint checks :

    @GET("/security/{companyId}/{subCompanyId}")
    @RolesAllowed("EDIT_COMPANY_{companyId}_{subCompanyId}")
    public String editSubCompany(String companyId, String subCompanyId){
        // ....
    }

Changes have been made to StdRestxSecurityManager in order to generate a role interpolation map, basically with query & path parameters (but which may be overwritten with specific stuff if wanted on every project .. you'll only need to @Provide another StdRestxSecurityManager)

By introducing variabilized roles, I added a support for wildcards on roles located in RestxPrincipal
That is to say, if current user has role named EDIT_COMPANY_1234_* he will be able to call every urls like /security/1234/* without having any 403 Forbidden response.
The wildcard roles checks are generated at compilation time through RestxAnnotationProcessor (because variable cartesian product may take some time to generate, which we would like to avoid at runtime).

That is to say, corresponding generated code for endpoint above will be :

        new StdEntityRoute<Void, java.lang.String>("default#SecuredResource#editSubCompany",
                readerRegistry.<Void>build(Void.class, Optional.<String>absent()),
                writerRegistry.<java.lang.String>build(java.lang.String.class, Optional.<String>absent()),
                new StdRestxRequestMatcher("GET", "/security/{companyId}/{subCompanyId}"),
                HttpStatus.OK, RestxLogLevel.DEFAULT) {
            @Override
            protected Optional<java.lang.String> doRoute(RestxRequest request, RestxRequestMatch match, Void body) throws IOException {
                securityManager.check(request, match, anyOf(hasRole("EDIT_COMPANY_*_*"), hasRole("EDIT_COMPANY_*_{subCompanyId}"), hasRole("EDIT_COMPANY_{companyId}_*"), hasRole("EDIT_COMPANY_{companyId}_{subCompanyId}")));
                return Optional.of(resource.editSubCompany(
                        /* [PATH] companyId */ match.getPathParam("companyId"),
                        /* [PATH] subCompanyId */ match.getPathParam("subCompanyId")
                ));
            }
            // ....
`
``
@@ -38,6 +36,8 @@
})
@SupportedOptions({ "debug" })
public class RestxAnnotationProcessor extends RestxAbstractProcessor {
private static final Pattern ROLE_PARAM_INTERPOLATOR_REGEX = Pattern.compile("\\{([^}]+)\\}");

This comment has been minimized.

@a-peyrard

a-peyrard Nov 11, 2015

Contributor

Isn't "\\{(.+?)\\}" more easy to read ?

This comment has been minimized.

@fcamblor

fcamblor Nov 15, 2015

Contributor

Agreed, added 4ce9ac1

@a-peyrard

This comment has been minimized.

Contributor

a-peyrard commented Nov 11, 2015

I'm ok with the introduction of this new feature.

I have some few remarks though.

  • shouldn't this PR be marked as breaking ?
  • with the breaking change of the Permission.has signature, it seems that Permission becomes linked with the StdRestxSecurityManager, any implementation of RestxSecurityManager has to be a subclass of StdRestxSecurityManager. What if the permission want to check something on the RestxRequest too ? So I wouldn't have changed the signature of Permission, but put the createRoleInterpolationMapFrom method in the permission generated by the Permissions.hasRole factory method.
  • by the way shouldn't we use DI to get the Permissions.hasRole, it might be useful to override the checking role process.

That been said I didn't used the security API very often, so maybe there is already enough existing anchors to change the security checking mechanism :)

@fcamblor

This comment has been minimized.

Contributor

fcamblor commented Nov 15, 2015

shouldn't this PR be marked as breaking ?

You're right, updated the PR title to reflect this

with the breaking change of the Permission.has signature, it seems that Permission becomes linked with the StdRestxSecurityManager, any implementation of RestxSecurityManager has to be a subclass of StdRestxSecurityManager.

I think I don't get what you mean. In what it is more linked to StdRestxSecurityManager than before ?
(note that initially, Permission.has was providing RestxRequest but it was never used in implementations that's why I found it interesting to decouple it from any restx.* classes)

What if the permission want to check something on the RestxRequest too ? So I wouldn't have changed the signature of Permission, but put the createRoleInterpolationMapFrom method in the permission generated by the Permissions.hasRole factory method.

In that case, currently, you just have to provide your own RestxSecurityManager implementation, subclassing StdRestxSecurityManager and overriding the createRoleIntepolationMap with your particular needs (in that method, you have access to RestxRequest).
That's what we're currently doing on my project where I need to interpolate "special" variables coming either from RestxRequest or on global application scope.

We may extract this behaviour into a dedicated @Component but I'm wondering if this is worth it as it will be a component strongly tied to StdRestxSecurityManager usage.

By the way shouldn't we use DI to get the Permissions.hasRole, it might be useful to override the checking role process.

That's what I expected initially when I started implementing the feature.

However after having played a bit with permissions API, I don't find it relevant for following reasons :

  • These methods are strongly coupled with the @RolesAllowed / @PermitAll annotations, and thus, with the annotation processor as well as it is used to "translate" annotation metadata to hasRole code portions in generated ResourceRouter
  • When looking at current Permissions utility methods, I don't really see fields where current implementations shouldn't be considered pertinent.
    At worst, you may want to add your own Permission implementation and in that case, you will either have to write your won ResourceRouter or have to register your own annotation processor, but I'm not sure this should be related to current PR scope.
  • If we would use @Component in place of different Permissions utility methods, we would have to inject this component through generated ResourceRouter class, in order to be able to use it in inlined StdEntityRoute declarations (like we're doing with securityManager component)

@fcamblor fcamblor changed the title from Variabilized security roles to [breaking] Variabilized security roles Nov 15, 2015

@a-peyrard

This comment has been minimized.

Contributor

a-peyrard commented Nov 20, 2015

So has you explained, everything is tied together, the annotation processor, the Permission creation, and the RestxSecurityManager (the interface as it uses a Permission in parameter).

So with this PR, for my POV you add a new link between StdRestxSecurityManager and Permission:

In that case, currently, you just have to provide your own RestxSecurityManager implementation, subclassing StdRestxSecurityManager

And as you said, if we want to override the mechanism, we will have to do either a subclass of StdRestxSecurityManager or to provide a custom annotation processor. I don't think it is very convenient to provide a custom annotation processor.

Again from my POV, I would like to be able to provide a PermissionFactory which will be used by the generated RestxRouter, and creates custom Permission. Like this, there will be no need to provide another RestxSecurityManager as the standard implementation is enough generic to work with almost any Permission implementation. And we might be able to use the annotation standard @RolesAllowed
annotation, values being used by the custom Permission or the PermissionFactory.

So I know that was not the goal of this PR, but by changing the Permission signature I think it will be harder to make something like that in the future.
As I was saying, I would not change the Permission signature (except the RestxRequestMatcher addition in parameter), and let the default implementation, used by the annotation processor Permission.hasRole, calculate the role interpolation map.

That being said, I just read the whole implementation one more time, and there is a problem in my proposal, it can not be the Permission.hasRole that calculate the interpolation map, otherwise the same map might be calculated multiple times. A solution would be to calculate it in Permission.anyOf, but this permission will not be able to use Permission anymore, but other objects, as it will have to give the map...

Anyway, I hope this message will be clearer than the previous one ;)

And again, I never had the need to change the default role management (yet, maybe in the next few month), so I will totally trust your judgement, if you think there is no need to keep the original API.

@fcamblor

This comment has been minimized.

Contributor

fcamblor commented Nov 20, 2015

mmm I'm not sure I'm getting it, let's bring some code on the table, I think it will be clearer :-)

Currently, annotation @RolesAllowed("foo", "bar_{oid}") will generate this line in the ResourceRouter

securityManager.check(request, /* match, <- added in this PR */ anyOf(hasRole("foo"), hasRole("bar_{oid}") /*, hasRole("bar_*") <- added in this PR */));

If I understand what you're meaning, you would like something like :

securityManager.check(request, permissionFactory.createPermissionsFor(request, match, "foo", "bar_{oid}"));

And it would be permissionFactory's responsibility to generate the HAS_ROLE[foo], HAS_ROLE[bar_12345] (interpolated oid) and HAS_ROLE[bar_*] role checks

Is that what you mean ?

@a-peyrard

This comment has been minimized.

Contributor

a-peyrard commented Nov 20, 2015

If I understand what you're meaning, you would like something like :
...
Is that what you mean ?

Almost ;)
Before the PR:

securityManager.check(request, permissionFactory.createPermissionsFor("foo", "bar"));

For a endpoint annotated with @RolesAllowed({"foo", "bar"}).

And with your PR, for a endpoint annotated with @RolesAllowed({"foo", "bar_{oid}"}):

securityManager.check(request, match, permissionFactory.createPermissionsFor("foo", "bar_{oid}", "bar_*"));

I would let the cartesian product being made by the annotation processor (like you said, its better to calculate all the roles during compile time), but the interpolation map base on the request and the match would be made by the Permission created by permissionFactory.

@fcamblor

This comment has been minimized.

Contributor

fcamblor commented Nov 20, 2015

Ok, so maybe something like :

securityManager.check(request, permissionFactory.interpolate(request, match, "foo", "bar_{oid}", "bar_*"));

permissionFactory will need both request and matcher to be able to interpolate things (see createRoleIntepolationMap)

Things I'm not a big fan here, is the fact that it is less "readable" than before :
anyOf( hasRole("foo"), hasRole("bar") ...) was easily understandable.

Here, we're adding the permissionFactory layer which is both allowing to plug things easily but decreases the readability of generated code.

I need to think about it a bit more

@a-peyrard

This comment has been minimized.

Contributor

a-peyrard commented Nov 20, 2015

For me the permission factory will not interpolate anything, it just creates a Permission looking like this:

public static Permission hasRole(final ImmutableList<String> roles) {
        return new Permission() {
            public final String TO_STRING = "HAS_ROLE[" + roles + "]";

            @Override
            public Optional<? extends Permission> has(RestxPrincipal principal, RestxRequest request, RestxRequestMatch match) {
                Map<String, String> roleMap = createRoleInterpolationMapFrom(request, match);

                for (String role : roles) {
                    // ... eventually return the permission...
                }
                return Optional.absent();
            }

            private Map<String, String> createRoleInterpolationMapFrom(RestxRequest request, RestxRequestMatch match) {
                // ...
            }

            @Override
            public String toString() {
                return TO_STRING;
            }
        };
    }
@a-peyrard

This comment has been minimized.

Contributor

a-peyrard commented Nov 20, 2015

And also I agree to say that it's less readable than before, in the generated router.

@fcamblor

This comment has been minimized.

Contributor

fcamblor commented Nov 20, 2015

ok got it, so we'd end with something like :

securityManager.check(request, match, anyOf(pf.hasRole("foo"), pf.hasRole("bar_{oid}"), pf.hasRole("bar_*")));

(we may improve readability by using a short var name like pf for the permission factory)

I understand now the problem about interpolation map being calculated on every has() call

I don't like the solution to make this calculation during anyOf() (nothing force the user to call it prior to hasRole()) and I don't like calling it in has() implementation either (for performance)

What about keeping interpolation map generation in StdRestxSecurityManager and passing the interpolation map instead of RestxRequest and RestxMatcher to the permissionFactory.hasRole().has() ? (this is what is currently done on the PR)

@fcamblor

This comment has been minimized.

Contributor

fcamblor commented Feb 13, 2016

As discussed, just updated the PR by refactoring Permissions utility class into a PermissionFactory restx @Component, thus allowing to override hasRole() implementation if willed.

However, I kept map interpolation at the StdRestxSecurityManager level.

Thus, line

securityManager.check(request, match, anyOf(hasRole("EDIT_COMPANY_*_*"), hasRole("EDIT_COMPANY_*_{subCompanyId}"), hasRole("EDIT_COMPANY_{companyId}_*"), hasRole("EDIT_COMPANY_{companyId}_{subCompanyId}")));

is now looking like :

securityManager.check(request, match, pf.anyOf(pf.hasRole("EDIT_COMPANY_*_*"), pf.hasRole("EDIT_COMPANY_*_{subCompanyId}"), pf.hasRole("EDIT_COMPANY_{companyId}_*"), pf.hasRole("EDIT_COMPANY_{companyId}_{subCompanyId}")));

Later I added some alias method in StdEntityRoute to improve readability, something like :

protected Permission hasRole(String role){
    return pf.hasRole(role);
}
// And so on for every PermissionFactory methods

It allows to bring back the initial

securityManager.check(request, match, anyOf(hasRole("EDIT_COMPANY_*_*"), hasRole("EDIT_COMPANY_*_{subCompanyId}"), hasRole("EDIT_COMPANY_{companyId}_*"), hasRole("EDIT_COMPANY_{companyId}_{subCompanyId}")));

generated code.

However, it brings some potential open doors to NPEs if developper calls by itself some constructors with null-PermissionFactor (I guess it should not happen frequently .. however some existing code using manual RestxRouter.Builder may compile with this change, and throw NPE if security checks are made after in the handmade doRoute impl)

@a-peyrard

This comment has been minimized.

Contributor

a-peyrard commented Apr 3, 2016

Sorry for the late response :/

I'm ok with your modifications.
With this new PR, we have two new cool stuffs, the parametrized roles and the ability to override the whole permissions system.

So feel free to merge.

fcamblor added some commits Oct 27, 2015

Allowing to pass RestxRequestMatcher to RestxSecurityManager [breaking]
This is intended to be able to use path parameters to interpolate 'dynamic' roles in the future

If you implemented your own RestxSecurityManager, you will have to update your check() prototype to take the
new RestxRequestMatcher parameter into consideration.
If you implemented your own routes relying on the securityManager.check() call, you will have to pass
the current restx request matcher to the secrutyManager.check() call.
Allowing to pass RestxRequestMatcher to Permission implementations [b…
…reaking].

If you provided your own Permission implementation(s), you will have to update your has() prototype method to include the RestxRequestMatcher
Interpolating roles with new roleInterpolationMap in Permissions.hasR…
…ole() implementation [breaking]

If you were using brackets ({}) in your roles, those will now be interpolated
Generating hasRole() check (at compilation time) for dynamic roles wi…
…th potential wildcard for every variable in role
Changed Permission.has() signature by replacing the request/requestMa…
…tcher with an interpolation map.

This interpolation map will be made of every query & path parameters
[breaking] Transformed Permissions utility class into a PermissionFac…
…tory component, thus allowing to potentially override hasRole() implementation if needed
[breaking] Provided some alises to permissionFactory in StdEntityRout…
…e allowing to have a more readable generated code in generated routes

Note that it implies StdEntityRoute requires PermissionFactory instance to be passed to the constructor.
A PermissionFactory-less has been kept for backward compatibility and Builder constructor, however it is discouraged to not provide it as calling
any of the method aliases will throw a NPE if permissionFactory is not set
[breaking] Considering a StdEntityRoute should *always* bring a permi…
…ssionFactory at construction time

Deprecated RestxRouter.Builder.addRoute() with no permissionFactory in order to avoid
NPEs when checking for security permissions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment