YarepGroup.addMember() not multi-threadable

[baszero]

Use-Case:

Last evening we had the rare situation where two different users did exactly the same thing.
The triggered action involved adding the user to a specific Yarep group.

My realm's code is using the group.addMember() method in order to assign a user to a specific group.

Error Description:

What happened was the following: Two threads tried to manipulate the .xml file via the standard Yarep API.
The group XML was corrupted after this and we had to manually repair the file.

Suggestion for improvement:

All methods that do physical changes in files should do this in a synchronized way.
But just applying synchronized(LOCK) in the addMember() method will not suffice, because when you save the file you must be sure that no other thread has already read the file as well. Two writes one after the other can overwrite their changes.

So my proposal would be to synchronized(LOCK) the addMember() method, and INSIDE the locked code I would reread the group, only then you can be sure that you are the only one reading the file for manipulation.

So the code could look like this:

private static final Object LOCK = new Object(); // The best way to have class locks!

public void addMember(Item item) throws AccessManagementException {
        if (null != item){
            if (item instanceof User) {
                memberUserIDs.add(item.getID());
                ((YarepUser) item).addGroup(getID());
                save();
            } else if (item instanceof Group) {
                if (wouldCreateGroupLoop(getID(), (Group)item)) {
                    // ...
                }
                // don't do this: memberGroupIDs.add(item.getID()); --> remove this line
                // Instead:
                synchronized(LOCK) {
                  Group g = ...; // Read group again
                  g.addParentGroup(getID()); // INFO: Add back/bi-directional link
                  g.save();
                }
            } else {
                log.warn("Item '" + item.getID() + "' is neither user nor group: " + item.getClass().getName());
            }
        } else {
            log.warn("Item is null. Can't add item (user or group) to the group '" + getID() + "'");
        }
    }

If you like, I can send a patch for this as an example? Please let me know.

Workaround:
The best workaround for this is to use synchronized blocks in the realm's code. Then the complexity of locking is transferred into the realm's code. But it would probably be easier for Yanel-Developers if they would not have to deal with this.

Source Code:

Link to method:
https://github.com/wyona/security/blob/master/src/impl/java/org/wyona/security/impl/yarep/YarepGroup.java#L191