4944 permission service bean cleanup #4998

michbarsinai · 2018-08-24T17:30:37Z

Related Issues

connects to PermissionServiceBean Cleanup #4944 PermissionServiceBean Cleanup

Pull Request Checklist

Unit tests completed
Integration tests: None
Deployment requirements, SQL updates, Solr updates, etc.: None
Documentation completed
Merged latest from "develop" branch and resolved conflicts

…b.com/IQSS/dataverse into 2122-querying-root-dataverse-contents

…now have their groupProvider set up properly

…ent, or even no statements at all, when the closure seed is empty

coveralls · 2018-08-24T17:39:30Z

Coverage decreased (-0.006%) to 15.595% when pulling 406fb41 on 4944-PermissionServiceBean-cleanup into 67d0094 on develop.

oscardssmith

Overall this looks great, The removal of 3000 lines of redundancy is quite nice. Some NotNull macros might clean stuff up a bit, and I think we can simplify groupservice bean with this, but I'll leave that up to you.

oscardssmith · 2018-08-24T20:46:53Z

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java

                    } catch (CommandException cex) {
-                        logger.warning("Problem getting rsync script (Command Exception): " + cex.getLocalizedMessage());
+                        logger.warning(()->"Problem getting rsync script (Command Exception): " + cex.getLocalizedMessage());


What do these changes do? It looks like it just creates clutter by adding anonymous functions. Is there a benefit?

There's a huge benefit in not creating the string unless it's actually going to be logged. Note that creating strings requires O(n) time and contiguous space, which means that the memory management and garbage collector have to work harder.
NetBeans actually displays a warning and offers an auto-fix when you use the previous version.

oscardssmith · 2018-08-24T20:49:24Z

src/main/java/edu/harvard/iq/dataverse/MapLayerMetadataServiceBean.java

@@ -109,14 +106,14 @@ public MapLayerMetadata findMetadataByDatafile(DataFile datafile){
        First check if the given user has permission to edit this data.

    */
-    public boolean deleteMapLayerMetadataObject(MapLayerMetadata mapLayerMetadata, User user){
+    public boolean deleteMapLayerMetadataObject(MapLayerMetadata mapLayerMetadata, DataverseRequest req){


Should this use @notnull? It will create compile time warnings if someone tries to pass null in.

The change I made in this method was replacing the user with the request, so that it uses the updated permission check system.

Adding @notNull would change the method behavior, so I suggest not adding it now, as it is not in scope, and would require additional testing (Also, the null -> false semantics might make sense).

There are other problems in this method (e.g. ignored result on line 121), but, again, not in scope for issue.

oscardssmith · 2018-08-24T20:49:58Z

src/main/java/edu/harvard/iq/dataverse/PermissionServiceBean.java

-                    .filter( Permission::requiresAuthenticatedUser )
-                    .collect( Collectors.toList() ));
-
+        EnumSet.copyOf(Arrays.asList(Permission.values()).stream()


Because we want the actual Set object to be an EnumSet, not just any set. This means set operations are done over bit of a long value, rather than over objects. Happy to change the method used, but I think that's the only one that does that.

oscardssmith · 2018-08-24T20:50:29Z

src/main/java/edu/harvard/iq/dataverse/PermissionServiceBean.java

+
+    /**
+     * A request-level permission query (e.g includes IP groups). These queries
+     * form a DSL for querying permissions, in a way that's hopefully more


What's a DSL?

Domain Specific Language. Updated the docs.

oscardssmith · 2018-08-24T20:51:39Z

src/main/java/edu/harvard/iq/dataverse/PermissionServiceBean.java

-        private StaticPermissionQuery(RoleAssignee user, DvObject subject) {
-            this.subject = subject;
-            this.user = user;
+    public RequestPermissionQuery on(DvObject d) {


Use @notnull here

oscardssmith · 2018-08-24T20:52:39Z

src/main/java/edu/harvard/iq/dataverse/PermissionServiceBean.java

+     * @param ipc Callback, called if there are insufficient permissions for the passed command.
+     * @return {@code true} iff the command will not be blocked due to permission issues.
+     */
+    boolean isPermitted(Command aCommand, InsufficientPermissionCallback ipc) {


Does anything use this anymore? If not, we should remove.

Yes, used twice (engine and internally in the bean).

oscardssmith · 2018-08-24T20:56:14Z

src/main/java/edu/harvard/iq/dataverse/authorization/groups/GroupProvider.java

@@ -46,6 +38,18 @@
     */
    public Set<T> groupsFor( RoleAssignee ra, DvObject dvo );


Can This go away now?

No. It's in use by the GroupServiceBean.

oscardssmith · 2018-08-24T20:56:28Z

src/main/java/edu/harvard/iq/dataverse/authorization/groups/GroupProvider.java

+     */
+    public Set<T> groupsFor( DataverseRequest req, DvObject dvo );
+
+    public Set<T> groupsFor( RoleAssignee ra);


No, it's used to get membership in groups that are not defined at a DvObject (e.g. AllUsers, AuthenticatedUsers, groups coming from shib, IP groups).

oscardssmith · 2018-08-30T00:55:04Z

src/main/java/edu/harvard/iq/dataverse/PermissionServiceBean.java

@@ -184,7 +187,7 @@ public RequestPermissionQuery on(DvObject dvo) {
     * @param d the object on which the permissions will be queried
     * @return A permission query
     */
-    public RequestPermissionQuery on(DvObject d) {
+    public RequestPermissionQuery on( @NotNull DvObject d) {
        if (d == null) {


d isn't null. that's why to NotNull it

Ideally, yes. But as far as I can tell, you still need to do runtime validation in regular Java (i.e. not using Lombok et al). See https://blogs.oracle.com/java-platform-group/java-8s-new-type-annotations (scroll down to the section "Optional Type Annotations are not a substitute for runtime validation").

It may generate compiler warnings (https://www.jetbrains.com/help/idea/nullable-and-notnull-annotations.html), but as long as there are no errors from standard javac, we have to do the check.

That said, I'll be very happy to see an official document saying we can remove that check :-)

It does say that "Your code should still perform runtime validation", but on the other hand, maven and any ide will not allow it to compile, at which point I feel like we should not have runtime checking for something we already check at compile time. It would be like testing isinstance every time we use generics because java's generics are broken

Dunno. This compiles on my machine with warning only:

public static void main(String[] args) { new PermissionServiceBean().on(null); }

(leaving the Java generics part for the next time we meet 😄)

…bean) that was in the original, "Oscar" branch, but not in the cleanup branch. (#4944)

pdurbin · 2019-03-20T03:13:03Z

@michbarsinai there are a fair number of merge conflicts.

dataversebot · 2019-08-15T14:08:45Z

Can one of the admins verify this patch?

pdurbin · 2019-08-21T17:58:04Z

This pull request is over a year old (currently our oldest pull request), has lots of merge conflicts, and hasn't seen any activity lately. Closing.

oscardssmith and others added 30 commits July 20, 2018 09:10

more speedups

8830927

removed duplicate check

5c27251

updated PermissionServiceBean

f7a9a43

removeall and retain all are kind of different

7676f69

temporary changes to add debugging

e425c5a

Checked in scripts for counting queries in the PostgresQL logs.

5b3cc46

groups now use TimeoutChache

d1d39fc

broken

f1b1d6c

Merge branch '2122-querying-root-dataverse-contents' of https://githu…

4586377

…b.com/IQSS/dataverse into 2122-querying-root-dataverse-contents

still broken

08fcd10

now it runs out of memory

730b219

5.8 seconds

28f9515

cleanup

18d798e

Merge branch 'develop' into 2122-querying-root-dataverse-contents

c025903

simplified permissionservicebean

3e228d6

minor cleanup

cb5be0d

merge

7ba270e

now it works (faster) without caching

50bbbe9

deleted cache classes and fix some named querries

1739ce2

code review feedback

ee0ef36

BugFix: Explicit groups loaded by loading a group that contains them …

ae46855

…now have their groupProvider set up properly

Explicit group transitive closure now works using a single SQL statem…

05b704b

…ent, or even no statements at all, when the closure seed is empty

Javadocs

2dd7ae8

Merge branch 'develop' into 4944-PermissionServiceBean-cleanup

b45467d

Removed static query, updated client code of the static queries

71f9a18

API cleanup in the PermissionServiceBean, and the client code

ac97bde

More cleanup of API

3151f64

A bit more cleanups

5ba3c74

Getting the documentation organized

f3f941b

Permission guide ready for QA

05d7c8e

michbarsinai added 4 commits August 24, 2018 12:29

(minor doc fix)

7f0fd1f

Encorporated the testable command guide into the developer guide

633d738

Added a footnote

9470b2d

Merged with dev

d7b009c

oscardssmith suggested changes Aug 24, 2018

View reviewed changes

michbarsinai added 2 commits August 30, 2018 02:37

Code review fixes

a80c37c

Merge branch 'develop' into 4944-PermissionServiceBean-cleanup

d6a4b15

oscardssmith reviewed Aug 30, 2018

View reviewed changes

Updated search fragment to use updated code

406fb41

michbarsinai mentioned this pull request Sep 3, 2018

Optimize Permissions Lookup #2122

Closed

pdurbin mentioned this pull request Sep 19, 2018

Running integration tests for more than just API #5068

Closed

pdurbin mentioned this pull request Oct 16, 2018

2122 querying root dataverse contents (and other permission performance boosts) #4883

Merged

1 task

Adding this one change (removing @NotNull attribute from a stateless …

1ee1ac9

…bean) that was in the original, "Oscar" branch, but not in the cleanup branch. (#4944)

pdurbin closed this Aug 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4944 permission service bean cleanup #4998

4944 permission service bean cleanup #4998

michbarsinai commented Aug 24, 2018

coveralls commented Aug 24, 2018 •

edited

Loading

oscardssmith left a comment

oscardssmith Aug 24, 2018

michbarsinai Aug 29, 2018

oscardssmith Aug 24, 2018

michbarsinai Aug 29, 2018

oscardssmith Aug 24, 2018

michbarsinai Aug 29, 2018 •

edited

Loading

oscardssmith Aug 24, 2018

michbarsinai Aug 29, 2018

oscardssmith Aug 24, 2018

michbarsinai Aug 29, 2018

oscardssmith Aug 24, 2018

michbarsinai Aug 29, 2018

oscardssmith Aug 24, 2018

michbarsinai Aug 29, 2018

oscardssmith Aug 24, 2018

michbarsinai Aug 29, 2018

oscardssmith Aug 30, 2018

michbarsinai Aug 30, 2018

oscardssmith Aug 30, 2018

michbarsinai Aug 30, 2018

pdurbin commented Mar 20, 2019

dataversebot commented Aug 15, 2019

pdurbin commented Aug 21, 2019

		@@ -46,6 +38,18 @@
		*/
		public Set<T> groupsFor( RoleAssignee ra, DvObject dvo );

4944 permission service bean cleanup #4998

4944 permission service bean cleanup #4998

Conversation

michbarsinai commented Aug 24, 2018

Related Issues

Pull Request Checklist

coveralls commented Aug 24, 2018 • edited Loading

oscardssmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michbarsinai Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdurbin commented Mar 20, 2019

dataversebot commented Aug 15, 2019

pdurbin commented Aug 21, 2019

coveralls commented Aug 24, 2018 •

edited

Loading

michbarsinai Aug 29, 2018 •

edited

Loading