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

Change security test to @RoleAllowed #836

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

Dev-ALPM
Copy link
Contributor

servletRequest.isUserInRole() return false with glassfish

Not test with Wildfly

Test before accept...

servletRequest.isUserInRole() return false with glassfish
@rsoika
Copy link
Member

rsoika commented Sep 28, 2023

Hi @Dev-ALPM ,
yes your changes work also in wildfly.
But my goal in some of these methods was to return a Response.Status.UNAUTHORIZED.
The @RoleAllowed results in a HTTP Error 500

hm....

You say you have problems with the isUserInRole method in glassfish. But this is often used in the core engine too.
Like here: https://github.com/imixs/imixs-workflow/blob/50d516d0e22cc253c669185bf614509268d727d5/imixs-workflow-engine/src/main/java/org/imixs/workflow/engine/DocumentService.java#L304-L311C3

This should work in glassfish as we run also projects in production with this app server.

Can you test if a construction like this works for you:

...
    	@Resource
	SessionContext ctx;
....

    @Path("/{uniqueid : ([0-9a-f]{8}-.*|[0-9a-f]{11}-.*)}")
    public Response deleteEntity(@PathParam("uniqueid") String uniqueid) {
        
        if (!ctx.isCallerInRole("org.imixs.ACCESSLEVEL.MANAGERACCESS")) {
            return Response.status(Response.Status.UNAUTHORIZED).build();
        }
        ItemCollection entity = documentService.load(uniqueid);
        if (entity != null) {
            documentService.remove(entity);
        }

        return Response.status(Response.Status.OK).build();
    }

Here I replace the servletRequest.isUserInRole() with ctx.isCallerInRole()

Also works with glassfish
@Dev-ALPM
Copy link
Contributor Author

Change with your proposal
Works well with glassfish

@rsoika
Copy link
Member

rsoika commented Sep 28, 2023

@Dev-ALPM This is great! I will merge it soon and did some more testing.

@rsoika rsoika merged commit de6dbb2 into imixs:master Sep 28, 2023
1 check failed
@rsoika rsoika added this to the 6.0.4 milestone Sep 28, 2023
@Dev-ALPM Dev-ALPM deleted the security-fix-glassfish branch September 28, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants