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.

Sign up for GitHub

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

Jump to bottom

IP group JSON should support single addresses #3340

Closed
pdurbin opened this issue Sep 9, 2016 · 7 comments
Closed

IP group JSON should support single addresses #3340

pdurbin opened this issue Sep 9, 2016 · 7 comments
Assignees
@kcondon
Milestone
4.5.1 - IP Groups...

Comments

@pdurbin
Copy link
Member

pdurbin commented Sep 9, 2016 •
edited
Loading

I gather from a336475 that IP groups should be able to be expressed as a single address. This should be tested/documented. I'm in the middle of resolving merge conflicts in pull request #3103 and I'm a little worried that I may be unfixing this? I'll leave a comment in a bit.
@pdurbin pdurbin added this to the 4.5.1 - IP Groups fixes milestone Sep 9, 2016
@pdurbin pdurbin self-assigned this Sep 9, 2016
pdurbin added a commit that referenced this issue Sep 9, 2016
@pdurbin
Merge branch 'develop' into 1380-honor-ip-groups #3103 #3273
80268c3 
Conflicts:
src/main/java/edu/harvard/iq/dataverse/api/Admin.java
src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java
src/test/java/edu/harvard/iq/dataverse/util/json/JsonParserTest.java

As of 1f5fcfe and earlier (not sure how far back) this branch cannot be
merged with develop since develop advanced with the release of 4.5 (pull
request #3308 especially was merged in).

A simple `git merge develop` yielded merge conflics, especially with
JsonPrinter.java and I was unable to resolve them using Netbeans. (I had
to run `git merge --abort` after three attempts.) I decided to revert
JsonPrinter.java to its state in 4.5 with
`git checkout dc58ae1
src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java` and
then manually copy the changes needed to get the app to compile from the
`1380-honor-ip-groups` branch (specifically
https://github.com/IQSS/dataverse/blob/2ada66142fd8fca3a61da4db46268cfba650e4a9/src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java
). I tried to get all the changes but it's possible I missed some.
`public static JsonObjectBuilder json( IpGroup grp )` especially had
some changes having to do with `List<List<String>> ranges =
grp.getRanges().stream().filter( rng -> !rng.isSingleAddress()` and
such. It looks like this change happened in a336475. The commit message
starts with "IP group json format now supports single addresses". I
opened #3340 to revisit this part of the code or at least to test and
document how it's supposed to work.

Meanwhile, I tested #3273 which is what I'm actually working on and the
"groups within groups" bug seems to be fixed both before and after the merge.
@pdurbin pdurbin mentioned this issue Sep 9, 2016
Closed
michbarsinai added a commit that referenced this issue Sep 10, 2016
@michbarsinai
IP ranges containing single IP appear as a single address rather than…
b0c0779 
… a range (solves #3340)
@michbarsinai
Copy link
Member

re: the documentation: I'm note sure we have a central place for documenting the formats (we should, of course, but I'm not sure we have the personnel).
We do have example son files at scripts/api/data/, and there ipGroup-localhost.json does have this format.

@pdurbin
Copy link
Member Author

pdurbin commented Sep 12, 2016

@michbarsinai thanks for the fix at b0c0779 ! I just moved this card to the QA column at https://waffle.io/IQSS/dataverse

pdurbin added a commit that referenced this issue Sep 12, 2016
@pdurbin
Merge branch '1380-honor-ip-groups' into mydata-ipgroups
5ec0cdf 
Merging the fix for #3340 (b0c0779)
@kcondon
Copy link
Contributor

kcondon commented Sep 12, 2016

This works but it requires that you specify both the ipv4 and ipv6 forms of the address, otherwise you get a 500 error. The range form allows only specifying ipv4, which is convenient.

Passing back to Michael for conisderation.

@kcondon kcondon removed their assignment Sep 12, 2016
@pdurbin pdurbin added ready and removed ready labels Sep 15, 2016
@kcondon
Copy link
Contributor

kcondon commented Sep 19, 2016

Found another aspect to this issue: in addition to not allowing just ipv4, the ranges version only works with ipv4. It allows both but then does not properly detect the incoming ip addr.

@michbarsinai
Copy link
Member

@kcondon I was not able to reproduce this (or, it's reverse day again). Here's what I did:

  1. Added two new groups, one containing a single IPv4 address and one containing a single IPv6 one (data/ipGroup-single-IPv4.json and data/ipGroup-single-IPv6.json). Addition was done via API.
  2. Added the localhost group (data/ipGroup-localhost.json).
  3. Added the all-addresses group (data/ipGroup-all.json).

Up to this point - no 500 errors.

Then I ran inclusion tests using the test API endpoint api/test/ipGroups/including/<address>. I got correct inclusion results for the IPs in the groups.

@michbarsinai michbarsinai assigned kcondon and unassigned michbarsinai Sep 20, 2016
@kcondon
Copy link
Contributor

kcondon commented Sep 20, 2016

@michbarsinai
All I did was

  1. try the localhost json but specify just my ip address, not localhost and only use the ipv4 part. It failed with an error.
  2. I tried the ipgroup-all and that worked in creating the group but then when I assigned a role to it, my machine/user did not get the perms. When I removed that group, then used the ipv4 version, perms were correctly granted, looking directly through ui, not using a test lib to check assignments

I can do a little more digging to more clearly identify steps if you want or if the above are not clear enough.

@kcondon
Copy link
Contributor

kcondon commented Sep 20, 2016

Tested all combinations and they all work! Closing.

@kcondon kcondon closed this as completed Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kcondon kcondon

Labels
None yet
Projects
None yet
Milestone
4.5.1 - IP Groups fixes
Development

No branches or pull requests
5 participants
@pdurbin @mheppler @michbarsinai @kcondon @djbrooke