Bugzilla – Full Text Bug Listing
| Summary: | [Security] Default user permission should be documented as minimum permission | ||
|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Pierre Gaufillet <pierre.gaufillet> |
| Component: | cdo.core | Assignee: | Project Inbox <emf.cdo-inbox> |
| Status: | CLOSED FIXED | QA Contact: | Christian Damus <give.a.damus> |
| Severity: | enhancement | ||
| Priority: | P2 | CC: | mathieu.velten, stepper |
| Version: | 4.3 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Mac OS X | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 415375 | ||
Hmm, I think permissions are/were meant to be additive in all cases. WRITE is already the highest permission, so that's why the method exits early. I've carefully tried to avoid as many expensive permission.isApplicable() calls as possible. The original purpose was really to optimize permission checking for special users with READ or WRITE access to everything. Thinking again it strikes me that it shouldn't be called defaultPermission but rather minimumPermission. Renaming a released feature/column is not an option, of course. What do you think? It is clearer with your explanation. If the feature cannot be renamed, maybe review the inline documentation? "default permission defines the minimum level of permission on all objects in the repository for a given user" or something like that, that appear when hovering on the property. I will add this precision also in the wiki article about security manager too. Moving all outstanding enhancements to 4.3 I can work this. I have created a new Gerrit review: https://git.eclipse.org/r/18053 This adds documentation in the model to clarify the semantics of the default access permissions and also adds a tooltip to the security editor to make sure that administrators understand it. The fix described in comment #5 has been merged to master. commit be46e5a42c02d941798cb9a6b7874885944230ff Closing. Closing. |
User default rights: in SecurityManager, I see: protected CDOPermission getPermission(CDORevision revision, CDORevisionProvider revisionProvider, CDOBranchPoint securityContext, User user) { CDOPermission result = convertPermission(user.getDefaultAccess()); if (result == CDOPermission.WRITE) { return result; } EList<Permission> allPermissions = user.getAllPermissions(); for (Permission permission : allPermissions) { CDOPermission p = convertPermission(permission.getAccess()); if (p.ordinal() <= result.ordinal()) ... if I understand correctly this code, it means that if the default access right for a given user is WRITE, then it replaces any other right that may have been defined elsewhere for this guy. It is disturbing, as most of the time default rights apply, well... when there is no other applicable right. I would therefore rather move this code to the end of the method and would not only return WRITE, but also READ and NONE access rights.