5
\$\begingroup\$

I have the following code where I try to manage user roles in teams:

Teams have entries, an entry has a creator. This creator is member of a team and has a specified role. Moderators may delete all entries, users may only delete their own entries. Here is how I implemented this:

public class TeamMember implements TeamRole {
    public String rank = "rank: Member";
    @Override
    public boolean deleteEntry(AbstractUser requester, Team team, Entry entry) throws IllegalAccessException {
        if(requester.getUid().equals(entry.getCreator().getUid())) {
            team.delete(entry);
            System.out.println("Member "  + requester.getName() +  " deleted own entry: " + entry.toString() + "from " + team.toString());
            return true;
        }
            throw new IllegalAccessException("Team members can only delete their own entries!");
    }

Moderator:

public class TeamModerator implements TeamRole {
    public String rank = "rank: Moderator";
    @Override
    public boolean deleteEntry(AbstractUser requester, Team team, Entry entry) throws IllegalAccessException {
        team.delete(entry);
        System.out.println("Moderator " +  requester.getName() + " deleted entry: "  + entry.toString() +  "from " + team.toString());
        return true;
    }

Team:

public class Team {
    private List<Entry> entries;
    private Map<AbstractUser, TeamRole> memberPerms;
    private String id;
    private String name;

public TeamRole findPermissionsForUser(AbstractUser user) {
    return memberPerms.get(user);
}
}

And the method that handles the request:

@RequestMapping(value = "/rest/entries/delete/userid/{userId}/teamid/{teamId}/entryid/{entryId}")
    public ResponseEntity<Team> deleteEntry(@PathVariable String userId, @PathVariable String teamId, @PathVariable String entryId) {
        Optional<Team> team = Repository.findTeamById(teamId);
        Optional<AbstractUser> user = Repository.findUserById(userId);
        Optional<Entry> entry = Repository.findEntryById(teamId, entryId);
        if (!(team.isPresent())) {
            return new ResponseEntity("team not found", HttpStatus.BAD_REQUEST);
        }
        if (!(user.isPresent())) {
            return new ResponseEntity("user not found", HttpStatus.BAD_REQUEST);
        }
        if (!(entry.isPresent())) {
            return new ResponseEntity("entry not found", HttpStatus.BAD_REQUEST);
        }
        return EntryService.deleteEntry(user.get(), team.get(), entry.get());

    }

The service that does the delete:

public class EntryService {
    public static ResponseEntity<Team> deleteEntry(AbstractUser requester, Team team, Entry e) {
        try {
            team.findPermissionsForUser(requester).deleteEntry(requester, team, e);
            return new ResponseEntity(team, HttpStatus.OK);
        } catch (IllegalAccessException e1) {
            e1.printStackTrace();
            return new ResponseEntity(e1.getMessage(), HttpStatus.INTERNAL_SERVER_ERROR);
        }
    }
}

This is just a MVCE which does not contain user authentication, it trustst that the passed userid is correct.
I think that this is not a good implementation, how would you model this?

\$\endgroup\$
  • 1
    \$\begingroup\$ Beware stringly typed code! \$\endgroup\$ – Konrad Rudolph Oct 9 '18 at 11:44
  • \$\begingroup\$ As stated, this is just an MVCE. The real application generates objects that contain all necessary information including authentication directly after taking the request with spring and pass them down to other methods. \$\endgroup\$ – ItFreak Oct 9 '18 at 12:00
4
\$\begingroup\$

I think that implementations of TeamRole should not know how to do things.

Based on the name part: ...Role it should only know if a certain user can perform a certain action on a particular resource.

I'm expecting the TeamRole to have a method like:

public interface TeamRole {
    boolean canDelete(Entry entry);
}

And then change the TeamMember:

public class TeamMember implements TeamRole {
    // need the user to be a part of the class here
    AbstractUser user;
    public TeamMember(AbstractUser user) {
        this.user = user;
    }

    @Override
    public boolean canDelete(Entry entry) {
        if (entry == null) return false;
        return user.getUid().equals(entry.getCreator().getUid());
    }
}

You can have an alternative with a static method in TeamMember, so you can avoid the instantiation of the class, and just pass the user (requester) as a parameter:

public interface TeamRole {
    // Java 8+
    static boolean canDelete(AbstractUser requester, Entry entry);
}

So your service method will become:

public static ResponseEntity<Team> deleteEntry(AbstractUser requester, Team team, Entry e) {
    TeamRole role = team.findPermissionsForUser(requester);
    if (role == null)
         return new ResponseEntity("no role found...", HttpStatus.INTERNAL_SERVER_ERROR);

    if (role.canDelete(entry)) {
        try {
            team.delete(e);
            return new ResponseEntity(team, HttpStatus.OK);
        } catch (Exception error) {
            // here you have an actual error to report to 
            // the user, as something that should be done
            // failed.
            return new ResponseEntity(error.getMessage(), HttpStatus.INTERNAL_SERVER_ERROR);
        }
    }
    // So you can say to the user that nothing is wrong 
    // with the request there is just a permission 
    // issue here.
    return new ResponseEntity("user cannot delete entity", HttpStatus.METHOD_NOT_ALLOWED);        
}

Profiling users is always an hard job, and each implementation should deal with the system requirements.

A good design should try to separate as much as possible the authorization part of the logic from the business part.

Authorization, like authentication and accounting should be aspect that can be changed in you system without adding any issue on the business logic.

Please have a look at Spring Security.

I don't mean you should use it, but I think could be a good inspiration on how things could be done cleaner.

Now a couple of things I noticed on your code.

The deleteEnytry method of TeamRole return a boolean, but this is always true!

So why returning such a value?

In this case just put void as the return value.

Have void as a return value from command method is considered a best practice. Of course is not mandatory and don't cover all the case, it is just usual that a coomand should not return something.

In your EntryService you have a NullPointerException:

public class EntryService {
    public static ResponseEntity<Team> deleteEntry(AbstractUser requester, Team team, Entry e) {
        try {         
            // NullPointer here!
            team.findPermissionsForUser(requester)
                .deleteEntry(requester, team, e);
            return new ResponseEntity(team, HttpStatus.OK);
        } catch (IllegalAccessException e1) {
            e1.printStackTrace();
            return new ResponseEntity(e1.getMessage(), HttpStatus.INTERNAL_SERVER_ERROR);
        }
    }
}

the team.findPermissionsForUser(requester) didn't return always a value, as it returns just the value found in the Map, that is an actual TeamRole or null if no entry was found in the map.

And as you catch just the IllegalAccessException you will got an error that could be complicated to debug.

\$\endgroup\$
  • \$\begingroup\$ good hint to not let the role know how to do the operation!:) \$\endgroup\$ – ItFreak Oct 9 '18 at 10:08
3
\$\begingroup\$

I would change this calls sequence

Optional<Team> team = Repository.findTeamById(teamId);
Optional<AbstractUser> user = Repository.findUserById(userId);
Optional<Entry> entry = Repository.findEntryById(teamId, entryId);
if (!(team.isPresent())) {
    return new ResponseEntity("team not found", HttpStatus.BAD_REQUEST);
}
if (!(user.isPresent())) {
    return new ResponseEntity("user not found", HttpStatus.BAD_REQUEST);
}
if (!(entry.isPresent())) {
    return new ResponseEntity("entry not found", HttpStatus.BAD_REQUEST);
}

As if there's no team - there's no sense in checking repository for user and entry. So It would be like this

Optional<Team> team = Repository.findTeamById(teamId);
if (!(team.isPresent())) {
    return new ResponseEntity("team not found", HttpStatus.BAD_REQUEST);
}
Optional<AbstractUser> user = Repository.findUserById(userId);
if (!(user.isPresent())) {
    return new ResponseEntity("user not found", HttpStatus.BAD_REQUEST);
}
Optional<Entry> entry = Repository.findEntryById(teamId, entryId);
if (!(entry.isPresent())) {
    return new ResponseEntity("entry not found", HttpStatus.BAD_REQUEST);
}
\$\endgroup\$
  • \$\begingroup\$ And the parentheses are extraneous. \$\endgroup\$ – xehpuk Oct 9 '18 at 12:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.