Bugzilla – Full Text Bug Listing
| Summary: | [Security] Simplified security model editor | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Christian Damus <give.a.damus> | ||||||
| Component: | cdo.ui | Assignee: | Christian Damus <give.a.damus> | ||||||
| Status: | CLOSED FIXED | QA Contact: | Eike Stepper <stepper> | ||||||
| Severity: | enhancement | ||||||||
| Priority: | P2 | CC: | alex.lagarde, stepper | ||||||
| Version: | 4.3 | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | Mac OS X | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | 419085 | ||||||||
| Bug Blocks: | 415375 | ||||||||
| Attachments: |
|
||||||||
I'll work on this. Created attachment 236186 [details]
Proposed visual editor design
Hi, Eike,
I have attached a shot of a partially functional prototype of a simplified "security manager" editor.
This editor supports only resource path-based filters in the roles.
The red arrows show how drag-and-drop associates roles with groups and users. Similar drag-and-drop gestures can associate roles, groups, and users in the detail forms for groups and users. Alternatively, each association list has an "Add..." button to add new associated elements from a list and a "New" button to create a new element to associate.
Undo/redo is, of course, fully supported in the usual way using the editing domain's commands (in many case via EMF's data bindings support).
Attribute fields (text, checkbox, combo box, etc.) show little "revert" buttons next to them on mouse hover to reset to default (not visible in the screenshot).
Warning decorations will be shown on UI controls that filter out unsupported model content (such as permission filters that aren't simple resource patterns). A warning banner message is also shown in such cases, directing the user to the generated model editor to manage the unsupported content.
I look forward to your feed-back.
Hi, Eike,
I have pushed a new branch committers/cdamus/security-editor with an initial implementation of my vision of a "simplified security editor" and invite you to play with it and provide some feed-back.
I quickly introduce its major features in this short video:
https://www.youtube.com/watch?v=XX87vpowm9M
I look forward to your comments! Thanks.
Ping! Hi, Eike, I am hoping that the editor implemented on this branch may be suitable for demonstration at ESE this year, as we discussed off-line. In the mean-time, I have a question relating to the security model: how does it interact with branching? As I understand it, a branch encompasses the entire repository (as in Git). That means that branches also branch the security model. But, then, the security model must be maintained in every branch for consistency. (I suppose it does enable use cases in which different branches actually have different security requirements, but I'm not sure that's a common problem) Am I missing some capability of the branching mechanism? Perhaps branches can cover only a subset of the repository such that the security model may remain unbranched? I have pushed a new branch committers/cdamus/passwords that integrates the work done for password management in bug 399306 (branch bugs/399306) into this new editor. This integration includes: * unifying the org.eclipse.emf.cdo.security.ui plug-in that both of these branches added * adding a "Reset Password..." button into the user details pane in the security form editor As always, feed-back is welcome! Hi, Eike, I have rebased the security-editor branch onto this morning's master (Eastern time) and squashed all of the commits into one, to make it easier to review. I have also cleaned up all of the copyright headers as discussed on bug 399306. I have pushed the result to a new branch: committers/cdamus/security-editor-1 And I have already created another new branch integrating the changes for bug 399306 (change/reset password functions) and this bug (security manager): committers/cdamus/passwords-1 This has the head of committers/cdamus/security-editor-1 rebased onto the head of bugs/399306 and follows it up with a third commit that adds a "Reset Password..." button into the details panel for Users in the Security Manager editor. OK, so now all of my committer branches mentioned in earlier comments are obsolete.
I have pushed a new branch
bugs/418452
that is based on the latest master, including the password management changes for bug 399306. This adds the new form-based security editor (as shown in the video referenced in comment #3) to the oee.cdo.security.ui plug-in introduced with bug 399306, complete with Reset Password... button in the details pane for users.
I have fixed the label-update bug in some of the tables that you can see in the video.
This is ready for code review.
The editor is very nice!!! Some comments: - Will the "some properties are not shown" warning eventually go away? - Why is a tiny warning icon in the lower left corner of the editor page? - Dropping a user/group onto users/groups in the detail pane of a role switches the detail pane to the dropped user/group. Is that intended? - The section headers look odd (a windows problem?) - In the absence of additional editor pages, do we need a page at all? - If the advanced editor can't be opened in a second editor page it should at least not replace the form editor. None of those should prevent you from merging the new tip of your branch ;-) Oh, I also added some questions in form of TODO markers in the code! commit 168480f024b1d66a920c99ca4ee92fb3ad81292d (In reply to Eike Stepper from comment #9) > The editor is very nice!!! I'm glad you like it! Thanks for taking time to look at so much code, especially on a Saturday morning! > Some comments: > - Will the "some properties are not shown" warning eventually go away? Hah. That's tricky. It will go away if the "Resource Tree (Reader|Writer)" roles are deleted, because they use EPackage filters in their permission, which the editor doesn't support. The problem is that these roles are created in the initialization of the security realm in a new repository! But, I think they aren't used by either the Administrator or a new user when one is created. So, we could possibly * discontinue creating these roles by default * add a quick-fix to the editor's warning that deletes them * ??? > - Why is a tiny warning icon in the lower left corner of the editor page? That shows a table or other widget that has an associated problem (hover on the warning decoration to see the problem). The warning in the page header summarizes all such problems. This facility is built into the Forms framework. > - Dropping a user/group onto users/groups in the detail pane of a role > switches the detail pane to the dropped user/group. Is that intended? Yes. The idea was that, after you drop, say, a role onto a user, you'd want to see the details of the user to view the context of that new role: what are all the roles now that the user has? I can easily be persuaded to change this. :-) > - The section headers look odd (a windows problem?) They look fine on Mac (as you can see in the video). Could you attach a screenshot? Or I can set up a development environment in Parallels. > - In the absence of additional editor pages, do we need a page at all? You're right. My original intent was to add the generated tree editor as another page, but that would require some refactoring in it to support sharing the editing domain, action-bar contributor, etc. I opted for the hokey Advanced editor button in the toolbar, instead. I should either do the refactoring to embed the tree editor or eliminate the multiple-page layout. What is your preference? > - If the advanced editor can't be opened in a second editor page it should > at least not replace the form editor. I was concerned about confusing users with possible interactions of changes between these editors. If they share a transaction but not an editing domain, then they see the same changes but undo/redo will conflict. If they use separate transactions, then the new editor won't see the changes made so far in the first editor. The best really would be the embedding as a second page. I'll just make that work. > None of those should prevent you from merging the new tip of your branch ;-) Hmmm. I'm not so sure. You have convinced me that this really isn't finished, yet! (In reply to Eike Stepper from comment #10) > Oh, I also added some questions in form of TODO markers in the code! Smart! (In reply to Eike Stepper from comment #9) > - Dropping a user/group onto users/groups in the detail pane of a role > switches the detail pane to the dropped user/group. Is that intended? Hmmm. I think I misread this and made no sense withy first reply. The behavior you describe is a bug. I'll look into it. (In reply to Christian W. Damus from comment #12) > Hah. That's tricky. It will go away if the "Resource Tree (Reader|Writer)" > roles are deleted, because they use EPackage filters in their permission, > which the editor doesn't support. The problem is that these roles are > created in the initialization of the security realm in a new repository! > But, I think they aren't used by either the Administrator or a new user when > one is created. Oh, I didn't realize that the warning was content-specific! > So, we could possibly > > * discontinue creating these roles by default > * add a quick-fix to the editor's warning that deletes them > * ??? What about only showing the warning when such a role is displayed in the details pane? > > - The section headers look odd (a windows problem?) > > They look fine on Mac (as you can see in the video). Could you attach a > screenshot? I meant to; and then forgot :P Will do in a minute. Created attachment 236928 [details]
Screenshot on Windows
(In reply to Eike Stepper from comment #14) > > > So, we could possibly > > > > * discontinue creating these roles by default > > * add a quick-fix to the editor's warning that deletes them > > * ??? > > What about only showing the warning when such a role is displayed in the > details pane? Possibly, but that's not really how the form editors are supposed to work. They are meant to implement essentially "live validation" (in the generic sense), presenting all of the problems currently manifest in the things you're editing. A practical problem is that the editor doesn't show these roles that have unsupported permission filters because it can't properly edit them. We would have to show them in some limited fashion in order to only show a warning on their details, but then the form editor framework will still put the summary warning banner at the top of the page. Maybe I could, instead, show these roles in some kind of greyed-out state with a details pane that is disabled. But how, then, to communicate to the user why these roles are not editable and that the advanced mode is required to edit them, if not via the problem decorations? > > > - The section headers look odd (a windows problem?) Thanks for the screenshot. AFAICS, it looks correct and, indeed, much as it does on Mac. I see a highlight scribbled over the following items: * problem summary banner: this is the standard presentation of Eclipse form editors. It looks correct * Groups section: the title and action buttons look OK to me What specifically is odd about these? Is it the level of decoration of the header? It's currently at a sort of medium trim level; it could be increased or eliminated. (In reply to Christian W. Damus from comment #13) > (In reply to Eike Stepper from comment #9) > > > - Dropping a user/group onto users/groups in the detail pane of a role > > switches the detail pane to the dropped user/group. Is that intended? > > Hmmm. I think I misread this and made no sense withy first reply. The > behavior you describe is a bug. I'll look into it. So, trying this scenario, I think I see what the problem is, but I'm not certain. Say I have a role Administration selected. I want to drop user cdamus into the Users list of the role. So, I grab cdamus in the Users section. Now the list in the Users section has a selection that it didn't have before, so it fires selection changed and the details pane of cdamus is shown so that the place I wanted to drag it to is no longer visible. Is this the problem you refer to? Now that the Users list has a selection, I can return to the Administration role, show its details, and drag cdamus from the Users section into Administration's list of users. This works because there is no selection change this time in the Users list. I'm not sure what could be done about this. Can the selection change behaviour somehow be blocked if a drag is initiated? Doesn't selection change as soon as the mouse goes down? It is generally easier to drag and drop between the lists on the left side: just drag the cdamus user onto the Administrator role or vice-versa to associate them. I have pushed new changes in four separate commits, as follows: commit e1c72e9: addressing specific questions left as TODOs in the code commit 8100313: show all roles in the Roles list, regardless of whether the editor can handle all of their permissions. The unsupported permissions are filtered from the Permissions table in the role's details, and a warning is only shown when a role is selected that has unsupported permissions. This ensures that (a) the warning banner is not shown on the editor header by default and (b) we still show warnings in context when the user needs to know there's missing information. commit 7fa8bfe: updates the "open advanced editor" action to open the new editor in the same EditingDomain as the current editor, so that we can leave the current editor open also. This then ensures consistency of editing and undo/redo in both editors. This relies on an enhancement in the CDOEditor to accept an EditingDomain associated with the editor input. commit bba430d: suppress the page tabs because we have only one page BTW, Eike, I noticed that you seem to have reformatted blank lines in several files, adding some here and removing others there. Do you have some additional code formatting settings that are not store in the project .settings/ folder? (In reply to Christian W. Damus from comment #18) > I have pushed new changes in four separate commits, as follows: Sorry, forgot to mention that these are pushed to the bugs/418452 branch only. The changes here are substantial enough that I would like them reviewed before merging everything to master. Thanks! > BTW, Eike, I noticed that you seem to have reformatted blank lines in
> several files, adding some here and removing others there. Do you have some
> additional code formatting settings that are not store in the project
> .settings/ folder?
I wondered when you would ask :P
The main rule is: Make it easy to recognize blocks of code that logically belong together.
That can for example mean: A block is declare+init a local variable, then use it, then empty line.
Lexically the are only 2 rules that the formatter can't enforce:
1. After a line that ends in } the next line must either also be } or empty. Ed found that }; is also allowed :P
2. After a line that ends in { no empty line must follow.
A little OCD, eh?!
(In reply to Eike Stepper from comment #20) > The main rule is: Make it easy to recognize blocks of code that logically > belong together. Hunh, yeah. I tend to be rather liberal with blank lines because I find that whitespace helps readability and to group logically statements that I think belong together. Trouble is, not everybody sees the same groupings ... :-) I have added these new rules to my CDO style guide. Thanks! > A little OCD, eh?! Sure. You may have noticed that I'm a little OCD about idioms like single return from methods. You may also begin to notice that I'm now fighting against my own nature in that department when working in the CDO code. I'm trying to fit in, but I have 20 years and 5 languages of experience to overcome! :-D I have pushed yet another commit to branch 418452, prompted by discussion on bug 399306. Commit 301b7a8: Added a 'userAuthenticated' property to the SessionProperties and used that to show the "Manage Security" action (which opens up the new editor) only on sessions that are authenticate, which would imply that there is some kind of security to manage. I have also merged additional commits for bug 399306 from master. Looks good. I've generified SelectionListenerAction and moved it to net4j.util.ui. It would be nice to have a session property that allows for testing administrative rights (realm writable by user?). What do you think? You can merge this commit to master, if you want: commit d16124e1aeec375a03244bbd9891911f351e4db9 (In reply to Eike Stepper from comment #23) > Looks good. I've generified SelectionListenerAction and moved it to > net4j.util.ui. Ah, cool. Good idea. > It would be nice to have a session property that allows for testing > administrative rights (realm writable by user?). What do you think? I had thought about that, but there's the question of how to find the realm. That is a potentially expensive operation (the security manager config can put the realm anywhere in the repo). We could have the security manager attach some new attribute to the repository info object, to indicate where the realm path is, but should that really be exposed to clients? Maybe only in a session that is logged in as the administrator. All other users would get no data, which is equally useful for the proposed session property. Which reminds me that I want to enhance the editor to search for the Realm if it isn't in the default location when the user opens it (the editor currently assumes the default realm path). The query facility should handle this easily. I'll do that in a follow-up commit. > You can merge this commit to master, if you want: > > commit d16124e1aeec375a03244bbd9891911f351e4db9 Thanks! > > It would be nice to have a session property that allows for testing
> > administrative rights (realm writable by user?). What do you think?
>
> I had thought about that, but there's the question of how to find the realm.
And in addition the write access to that can change at any time. We can postpone this issue ;-)
(In reply to Eike Stepper from comment #23) Hi, Eike, Just to be clear, do you mean that you don't want your follow-up commit 0f604ce merged to master? The one with the useful refactoring of the SelectionListenerAction? > You can merge this commit to master, if you want: > > commit d16124e1aeec375a03244bbd9891911f351e4db9 The from-based security editor has been merged to master. Thanks, Eike, for all of your help! commit e611eddd1b72f409e40017ef86403b169a421e2f I copied the wrong commit ID. You did it all right! Thanks ;-) Closing. Closing. |
The current security model editor is the EMF-generated tree editor, with some customizations (such as suppressing the UserPassword object from the tree viewer). This offers, of course, the most complete control over the security model, supporting every conceivable advanced use case. Like most generated editors, it doesn't do much to help users to understand how to model the most common uses cases. Users need to be able to think in terms of the specific semantics of the model, not just what they want users to be able to do and not be able to do in the repository. This enhancement proposes a more simplified editor with a friendly forms-based interface that makes it obvious how to implement the most common use cases of: * administrators: can edit anything. Administrator-ness is defined as having write access to the security realm resource * regular users: restricted to editing certain resource nodes * choose which resources users have access to visually, with and without sub-trees * one-off roles for user-specific permissions * one-click support for the new-in-4.3 "home folder" access * drag-and-drop gestures for associating roles, users, and groups with one another By limiting the configuration of access permissions to resource patterns, traditional ACL use cases are easily implemented. For schema-based and object-level permissions, users will have to employ the generated tree editor. This will be included as a second page in the editor (in the usual multi-page editor fashion). If the security model includes any advanced constructs not supported by the simplified view, a warning banner will be shown that directs the user to this page to manage them there. This editor will use the new "Change Password" support in bug 399306 to manage user passwords.