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

DS-3572: Check authorization for a specified user instead of currentUser #1715

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@pnbecker
Copy link
Member

commented Apr 18, 2017

@pnbecker pnbecker changed the base branch from master to dspace-6_x Apr 18, 2017

@pnbecker pnbecker added this to the 6.1 milestone Apr 18, 2017

@pnbecker

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2017

We should port this to our master branch as well.

@tdonohue

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

@pnbecker : strangely, Travis seems to have failed to kickoff a CI build on this branch/PR. Could you try and either recreate this PR or push up a minor change (which you can then revert) to kickstart Travis?

@pnbecker pnbecker force-pushed the tuub:DS-3572 branch to 33a3d53 Apr 19, 2017

@pnbecker

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2017

@tdonohue I rebased my PR, this should trigger TravisCI. Thank you for notifying.

@terrywbrady

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2017

@pnbecker , +1 on the code change. Do you have any advice on testing/verifying this change?

@pnbecker

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

@terrywbrady Probably it would be good to add some unit tests that check this. 🙄

The only way I know to test this, is the unit test I've written against DS-3431.

@pnbecker

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2017

The unit test is not working as expected, I mark that as WIP.

@tomdesair
Copy link
Contributor

left a comment

I've suggested some changes.

That method in the AuthorizeService was introduced by this commit: 9817028

So it seems we can test this by using JSPUI RequestItemServlet. I'm not a JSPUI expert but it seems that we can test this like this:

  • Create a private item with a private bitstream both limited to a certain group.
  • As an anonymous user, request access to that item AND its bitstreams.
  • As an admin (or as a user member of the above group), approve the item request.

--> I expect that the private bitstream of that item is attached even though they are not available to the anonymous group because the authorizeService.authorizeActionBoolean(context, null, bitstream, Constants.READ, false) call will look at the membership of the current user and not of the anonymous user.

@@ -309,7 +309,7 @@ protected boolean authorize(Context c, DSpaceObject o, int action, EPerson e, bo
}

if ((rp.getGroup() != null)
&& (groupService.isMember(c, rp.getGroup())))
&& (groupService.isMember(c, e, rp.getGroup())))

This comment has been minimized.

Copy link
@tomdesair

tomdesair Apr 27, 2017

Contributor

We would also need to add the userToCheck variable to the isAdmin(c, adminObject) call on line 257

This comment has been minimized.

Copy link
@pnbecker

pnbecker Jun 2, 2017

Author Member

good catch! thanks.

This comment has been minimized.

Copy link
@pnbecker

pnbecker Jun 6, 2017

Author Member

should be solved now.

}
}
//lookup eperson in normal groups and subgroups
return epersonInGroup(context, groupName, currentUser);

This comment has been minimized.

Copy link
@tomdesair

tomdesair Apr 27, 2017

Contributor

I would leave the epersonInGroup method in place as this was written especially to make the isMember method more performant (see ticket https://jira.duraspace.org/browse/DS-3004): Saving the retrieval of the Group object from the database and checking membership with 1 query so that it's easily cacheable by Hibernate.

I think it is too expensive to retrieve all the groups of an eperson (multiple times) for each web request if not absolutely necessary (read: when there are no special groups).

This comment has been minimized.

Copy link
@pnbecker

pnbecker Jun 2, 2017

Author Member

I'll take a look next week.

@pnbecker pnbecker force-pushed the tuub:DS-3572 branch 2 times, most recently from c02a9b8 to fa2efcc Jun 2, 2017

@pnbecker

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2017

As first action on this PR I fixed the test. No it fails when the PR is not in place and succeeds when the PR is.

This PR needs to be rebased. Some caching was added to org.dspace.eperson.GroupServiceImpl. The method affected by the merge conflict is exactly the part of this PR that needs to be improved. Nevertheless, as the code in this PR and the current code in our dspace-6_x branch can be part of the soultion, I don't rebase that now, but leave it to when this PR is overhauled.

@pnbecker pnbecker force-pushed the tuub:DS-3572 branch from fa2efcc to bb1e13a Jun 6, 2017

@pnbecker

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2017

I rebased this and updated my fix for the issue. The test currently is failing. I think the culprit is GroupService.isMember(context, eperson, group) or GroupDAOImpl.findByNameAndMembership(Context, String, EPerson).

@tomdesair Could you please take a look if you see any reason why the test still fails? Are you sure GroupDAOImpl.findByNameAndMembership(Context, String, EPerson) works as expected? Can you please give it another review?

@pnbecker

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2017

Closing this PR in favor for #1767

@pnbecker pnbecker closed this Jun 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.