Object oriented design for access control

Question

Problem Statement:

Design an employee swap in swap out system. The system will have a machine which records the swap in and swap out. The user can also login in a portal where he can check his swap in /swap out time. He can correct his time also.

There will be managers for employee who can check the entry for all the employees which are under them and correct their subordinates timings also.

Similarly, a HR person can only view every Employee's records.

I chalked out the following skeleton. I am new to design and would like any pointers on how to approach such problems.

class User{
    String name;
    String designation;
    int id;
}

class AccessGroup{
    int priority;
    String name;
    Boolean read;
    Boolean write;
    //Setter - Getter methods
}

static class AccessManagement{
    HashSet<String, PriorityQueue<AccessGroup> accessControlTable;

    public void addUser(User u, AccessGroup g);
    public void addUserToGroup(User u, AccessGroup g);
    public void removeUser(User u);
    public void removeUserFromGroup(User u, AccessGroup g);
    public AccessGroup getPriorityAccessGroupForUser(User u);
    public PriorityQueue<AccessGroup> getUserGroups(User u);
    public boolean isValidUser(User u);
    public boolean hasReadPermission(User u);
    public boolean hasWritePermission(User u);
}

class Mediator{
    String url="";

    public List<Records> getRecords(User u);
    public void updateRecordForUser(User u, Record r, Value v){
        if(!AccessManagement.isValidUser(u))
            throw Exception();

        if(AccessManagement.hasWritePermission(u)){
            try{
                Connection conn = DriverManager.getConnection(url);
                Statement statement = conn.createStatement();
                statement.executeQuery("Update r in TimeStamps where id= u.id");
            }
            catch(Exception e){
                //Do something
            }        
        }        
    }
}

Answer 1

• AccessGroup should contain a Set of permissions to be extensible, rather than have the Read permission and Write permission directly attached. Otherwise you end up with a dozen attributes hanging off AccessGroup in a year's time.
• (this also means you can just union the permissions on a user to see what they can do, instead of messing with AccessGroup.getXXXX attributes)
• You're overloading the terms 'add' and 'remove'. You should createUser and deleteUser
• The User class is quite anemic. Perhaps you could use composition to hide the entire AccessManagement class inside it, maybe by having an AuthenticatedUser subclass that knows about its groups and permissions.
• Your mediator should have specific methods to swapIn and swapOut. TBH I'd model it as an event stream so there would be no updates, only inserts.

Answer 2

Welcome to Code Review, and thanks for sharing your code! Altogether, your program looks pretty good. It's nice and short, you have good indentation, all your variables have reasonable names, and it's very easy to read. A couple things to fix and work on in the future:

1. Always use braces in if statements.

It looks nicer, prevents bugs, increases readability, and there is no down-side to it. So change this statement:

    if(!AccessManagement.isValidUser(u))
        throw Exception();

to this:

    if(!AccessManagement.isValidUser(u)) {
        throw Exception();
    }

2. Missing >.

    HashSet<String, PriorityQueue<AccessGroup>> accessControlTable;
                                             ^

3. Make use of the keyword throws.

Ahhhh yes... Back to this statement again. This could be removed altogether with a simple line in your header. Rather than this:

    if(!AccessManagement.isValidUser(u)) {
        throw Exception();
    }

Just do this:

    public void updateRecordForUser(User u, Record r, Value v) throws Exception {
        if(AccessManagement.hasWritePermission(u)){
            try{
               //...code down here

That's just about all I could think of. Keep up the good coding!