You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
privatestaticfinalObjectLOCK = newObject(); // The best way to have class locks!publicvoidaddMember(Itemitem) throwsAccessManagementException {
if (null != item){
if (iteminstanceofUser) {
memberUserIDs.add(item.getID());
((YarepUser) item).addGroup(getID());
save();
} elseif (iteminstanceofGroup) {
if (wouldCreateGroupLoop(getID(), (Group)item)) {
// ...
}
// don't do this: memberGroupIDs.add(item.getID()); --> remove this line// Instead:synchronized(LOCK) {
Groupg = ...; // Read group againg.addParentGroup(getID()); // INFO: Add back/bi-directional linkg.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.
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:
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
The text was updated successfully, but these errors were encountered: