Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add groupsFor(AuthenticatedUser au) method #3055 #3062

Merged
merged 4 commits into from
May 17, 2016

Conversation

pdurbin
Copy link
Member

@pdurbin pdurbin commented Apr 5, 2016

RFI Checklist

1. Related Issues


2. Pull Request Checklist

  • Functionality completed as described in FRD
  • Dependencies, risks, assumptions in FRD addressed
  • Unit tests completed
  • Deployment requirements identified (e.g., SQL scripts, indexing)
  • Documentation completed
  • All code checkins completed

3. Review Checklist

After the pull request has been submitted, fill out this section.

  • Code review completed or waived
  • Testing requirements completed
  • Usability testing completed or waived
  • Support testing completed or waived
  • Merged with develop branch and resolved conflicts

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 5.012% when pulling ea89841 on 3055-get-more-groups into 90cb6da on develop.

if (identifierWithoutAtSign != null) {
roleAssigneeSvc.getUserGroups(identifierWithoutAtSign).stream().forEach((groupAlias) -> {
ExplicitGroup explicitGroup = explicitGroupProvider.get(groupAlias);
if (explicitGroup != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the full lambda experience, you can use map to map the group alias to an explicit group, filter out the nulls, and then use a collector to get the set of groups.

@pdurbin
Copy link
Member Author

pdurbin commented Apr 6, 2016

@scolapasta I'm passing this pull request to you to decide whether to merge it or not since @kcondon has already tested the code and closed the issue it fixes (#3055).

@pdurbin
Copy link
Member Author

pdurbin commented May 13, 2016

Hmm, this pull request has merge conflicts now in src/main/java/edu/harvard/iq/dataverse/RoleAssigneeServiceBean.java

It looks like getUserGroups was renamed to getUserExplicitGroups in 56f14ee

I made this simply change but testAssignGroupAtDataverse isn't passing:

INFO: expected: [group_user22, group_26-groupFor2d1fc9dd]
INFO: actual: [group_user24, group_26-groupFor2d1fc9dd]

Not sure what's going on here or what the priority of #3055 is.

@scolapasta scolapasta assigned pdurbin and unassigned scolapasta May 13, 2016
@scolapasta
Copy link
Contributor

Tested pre merge code with @pdurbin , saw similar issue, so seems unrelated to merge.

@pdurbin
Copy link
Member Author

pdurbin commented May 16, 2016

I just replicated the problem Gustavo saw. I switched to 3055-get-more-groups, didn't attempt to merge, and the test fails:

May 16, 2016 1:48:43 PM edu.harvard.iq.dataverse.api.SearchIT testAssignGroupAtDataverse
INFO: expected: [group_28-groupFor4074823b, group_user146]
May 16, 2016 1:48:43 PM edu.harvard.iq.dataverse.api.SearchIT testAssignGroupAtDataverse
INFO: actual: [group_user122, group_28-groupFor4074823b]

I'm not sure what's going on but I'll spend an hour or so trying to figure it out.

@pdurbin
Copy link
Member Author

pdurbin commented May 16, 2016

I just dropped my database with scripts/database/homebrew/delete-all, recreated it by deploying the war file, and got set up again with scripts/database/homebrew/rebuild-and-test and now the testAssignGroupAtDataverse test passes. I'm a bit concerned that I was previously (before dropping my database) seeing #2418 about mismatched IDs in the database but not sure yet.

Conflicts:
src/main/java/edu/harvard/iq/dataverse/RoleAssigneeServiceBean.java

Here are the changes I had to make:

- Make RoleAssigneeServiceBean.getUserExplicitGroups public.
- Stop using old getUserGroups name.

Also, SearchIT better reflects reality in terms of which tests can run
successfully at this time.
@pdurbin
Copy link
Member Author

pdurbin commented May 16, 2016

These are the conflicts I saw when merging "develop" (7fe7040) into "3055-get-more-groups" (ea89841):

murphy:dataverse pdurbin$ git merge develop
Auto-merging src/test/java/edu/harvard/iq/dataverse/api/UtilIT.java
Auto-merging src/test/java/edu/harvard/iq/dataverse/api/SearchIT.java
Removing src/main/webapp/apitoken.xhtml
Auto-merging src/main/java/edu/harvard/iq/dataverse/search/SearchServiceBean.java
Auto-merging src/main/java/edu/harvard/iq/dataverse/RoleAssigneeServiceBean.java
CONFLICT (content): Merge conflict in src/main/java/edu/harvard/iq/dataverse/RoleAssigneeServiceBean.java
Auto-merging doc/sphinx-guides/source/_static/installation/files/etc/httpd/conf.d/dataverse.example.edu.conf
Automatic merge failed; fix conflicts and then commit the result.
murphy:dataverse pdurbin$ 

Here are the changes I had to make (see 3cc98db):

  • Make RoleAssigneeServiceBean.getUserExplicitGroups public.
  • Stop using old getUserGroups name.
murphy:dataverse pdurbin$ git diff
diff --git a/src/main/java/edu/harvard/iq/dataverse/RoleAssigneeServiceBean.java b/src/main/java/edu/harvard/iq/dataverse/RoleAssigneeServiceBean.java
index b899893..d632c31 100644
--- a/src/main/java/edu/harvard/iq/dataverse/RoleAssigneeServiceBean.java
+++ b/src/main/java/edu/harvard/iq/dataverse/RoleAssigneeServiceBean.java
@@ -274,7 +274,11 @@ public class RoleAssigneeServiceBean {
         return " AND r.definitionpoint_id IN (" + StringUtils.join(outputList, ",") + ")";
     }

-    private List<String> getUserExplicitGroups(String roleAssigneeIdentifier) {
+    /**
+     * @todo Support groups within groups:
+     * https://github.com/IQSS/dataverse/issues/3056
+     */
+    public List<String> getUserExplicitGroups(String roleAssigneeIdentifier) {

         String qstr = "select  groupalias from explicitgroup";
         qstr += " where id in ";
diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/groups/GroupServiceBean.java b/src/main/java/edu/harvard/iq/dataverse/authorization/groups/GroupServiceBean.java
index 602aa5d..55af770 100644
--- a/src/main/java/edu/harvard/iq/dataverse/authorization/groups/GroupServiceBean.java
+++ b/src/main/java/edu/harvard/iq/dataverse/authorization/groups/GroupServiceBean.java
@@ -126,7 +126,7 @@ public class GroupServiceBean {
             }
         }
         if (identifierWithoutAtSign != null) {
-            roleAssigneeSvc.getUserGroups(identifierWithoutAtSign).stream().forEach((groupAlias) -> {
+            roleAssigneeSvc.getUserExplicitGroups(identifierWithoutAtSign).stream().forEach((groupAlias) -> {
                 ExplicitGroup explicitGroup = explicitGroupProvider.get(groupAlias);
                 if (explicitGroup != null) {
                     groups.add(explicitGroup);
murphy:dataverse pdurbin$ 

On a clean, new database the test passed:

May 16, 2016 2:29:42 PM edu.harvard.iq.dataverse.api.SearchIT testAssignGroupAtDataverse
INFO: expected: [group_21-groupFord1330ba1, group_user19]
May 16, 2016 2:29:42 PM edu.harvard.iq.dataverse.api.SearchIT testAssignGroupAtDataverse
INFO: actual: [group_21-groupFord1330ba1, group_user19]

Last week I showed @scolapasta how to hack SearchIT.java into running just the test I wanted to run (testAssignGroupAtDataverse) but this was ridiculous. In 3cc98db I added a todo at the top to indicate that the SearchIT tests are not working and added a bunch of @Ignore's. I'd like to suggest that we work on this some day as part of #2460.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 5.143% when pulling 3cc98db on 3055-get-more-groups into 7fe7040 on develop.

@pdurbin
Copy link
Member Author

pdurbin commented May 16, 2016

@scolapasta as described in my last comment at #3062 (comment) I merged "develop" into this pull request and got the "testAssignGroupAtDataverse" test we were looking to pass but this required me dropping my database. Why? I think there's something odd going on with mismatched user IDs, as I originally reported at #2418. To get the test to fail again, (to make my database unclean?) all I have to do run the "AdminIT.java" tests which converts users from builtin to shib and back again. Then "testAssignGroupAtDataverse" from SearchIT.java fails and "testUserId" from BuiltinUsersIT.java fails as well. Here's the javadoc i wrote there:

/**
 * This test is expected to pass on a clean, fresh database but for an
 * unknown reason it fails when you load it up with a production
 * database from dataverse.harvard.edu. Why? This is what
 * https://github.com/IQSS/dataverse/issues/2418 is about.
 */
assertEquals(userIdFromDatabase, userIdFromJsonCreateResponse);

@scolapasta I'm passing this to you to see what you think.

@pdurbin pdurbin assigned scolapasta and unassigned pdurbin May 16, 2016
@pdurbin
Copy link
Member Author

pdurbin commented May 17, 2016

@scolapasta it turns out the mismatched user ID problem was simply me not understanding which ID I was getting back in the JSON response when creating a builtin user. In pull request #3124 I'm suggesting that we should return both the builtin user id and the authenticated user id (and more). If you'd like I could merge the branch for that pull request into the branch for this one and then clean up SearchIT.java to get the ID from the authenticated user or the builtin user. This way the test will always pass, regardless of if you have Shib users in your database or not.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.008%) to 5.137% when pulling 766cca4 on 3055-get-more-groups into 5c6cc8e on develop.

@pdurbin
Copy link
Member Author

pdurbin commented May 17, 2016

#3124 got merged so in 766cca4 I updated testAssignGroupAtDataverse to work even when you have Shib users in your database.

@scolapasta scolapasta added this to the 4.4 milestone May 17, 2016
@scolapasta scolapasta merged commit 235eaa6 into develop May 17, 2016
@pdurbin pdurbin deleted the 3055-get-more-groups branch May 20, 2016 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants