Skip to content

optionally split Shibboleth affiliation and return first or last value#8880

Merged
pdurbin merged 27 commits intoIQSS:developfrom
DataverseNO:dvno-Affiliation
Sep 9, 2022
Merged

optionally split Shibboleth affiliation and return first or last value#8880
pdurbin merged 27 commits intoIQSS:developfrom
DataverseNO:dvno-Affiliation

Conversation

@Louis-wr
Copy link
Contributor

@Louis-wr Louis-wr commented Aug 2, 2022

What this PR does / why we need it: Feide authentication service returns an array of values. The affiliation in English is expected to be the last element in the array. This modification adds an option to only return the last element of said array.

Which issue(s) this PR closes:

Closes #8882

Special notes for your reviewer:

Suggestions on how to test this: the array is comma separated for example :
"UiT Norges Arktiske Universitet,UiT The Arctic University of Norway"
The separator is set with the following curl command :
curl -X PUT -d "," http://localhost:8080/api/admin/settings/:ShibAffiliationSeparator

The last value is selected using :
curl -X PUT -d "lastAffiliation" http://localhost:8080/api/admin/settings/:ShibAffiliationOrder
will make it so the affiliation is only "UiT The Arctic University of Norway"

The first value is selected using :
curl -X PUT -d "firstAffiliation" http://localhost:8080/api/admin/settings/:ShibAffiliationOrder
will make it so the affiliation is only "UiT Norges Arktiske Universitet"

Does this PR introduce a user interface change? If mockups are available, please link/include them here: NO

Is there a release notes update needed for this change?: YES
New parameter in configuration : :ShibAffiliationOrder
New parameter in configuration : :ShibAffiliationSeparator

Additional documentation:
under database settings
:ShibAffiliationOrder
Returns the last value of an array in affiliations list.
The array is separated using :ShibAffiliationSeparator
curl -X PUT -d "lastAffiliation" http://localhost:8080/api/admin/settings/:ShibAffiliationOrder

:ShibAffiliationSeparator
Set the separator for the affiliation array
Default : ";"
curl -X PUT -d ";" http://localhost:8080/api/admin/settings/:affiliationSeparator

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

@Louis-wr thanks for the PR! Overall, this looks pretty good but I commented about some TODOs. Please let me know if you have any questions.

@Louis-wr
Copy link
Contributor Author

Louis-wr commented Aug 2, 2022

@pdurbin Thank you for your comments. I think i solved all the issues, nevertheless please feel free to get back to me if more changes are required.
I took the liberty of using the name doc/release-notes/8880-Dvno-affiliation.md so it will match the documentation and other pull request, please let me know if you had something else in mind.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Hi @Louis-wr much better! Thanks! I noticed a few more things. Please see my additional comments.

Louis-wr and others added 3 commits August 2, 2022 15:48
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
placed .. _:ComputeBaseUrl: at the proper location
@Louis-wr Louis-wr mentioned this pull request Aug 2, 2022
@Louis-wr
Copy link
Contributor Author

Louis-wr commented Aug 2, 2022

@pdurbin I took care of the issues you pointed out. I am not sure what went wrong with the indentations but it should look a lot more consistent now.

@pdurbin pdurbin changed the title Dvno affiliation optionally split Shibboleth affiliation and return first or last value Aug 3, 2022
@scolapasta
Copy link
Contributor

Hi @Louis-wr , for this next sprint we are catching up on Community PRs. Would you mind updating/ refreshing thiis PR from develop? Thanks!

@Louis-wr
Copy link
Contributor Author

Louis-wr commented Aug 4, 2022

@scolapasta done, let me know if you need anything else.

@coveralls
Copy link

coveralls commented Aug 25, 2022

Coverage Status

Coverage decreased (-0.007%) to 20.035% when pulling c62deaf on DataverseNO:dvno-Affiliation into f9c1a02 on IQSS:develop.

@pdurbin
Copy link
Member

pdurbin commented Aug 25, 2022

Huh. I clicked the button to run the API tests (they don't run automatically for first-time pull request authors) and HarvestingServerIT.testOaiFunctionality is failing according to https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-8880/17/testReport/junit/edu.harvard.iq.dataverse.api/HarvestingServerIT/testOaiFunctionality/

I know @Louis-wr (and anyone off campus) can't see Jenkins due to a firewall but here's a screenshot:

Screen Shot 2022-08-25 at 9 14 47 AM

Let me ask about re-running the tests or maybe kick off a new run myself.


UPDATE: We're seeing this test failing on the develop branch as well. As I suspected, this failure is unrelated to this pull request. All the other API tests for this pull request succeeded.


UPDATE 2: I opened an issue for the harvest test failure:

@Louis-wr
Copy link
Contributor Author

Huh. I clicked the button to run the API tests (they don't run automatically for first-time pull request authors) and HarvestingServerIT.testOaiFunctionality is failing according to https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-8880/17/testReport/junit/edu.harvard.iq.dataverse.api/HarvestingServerIT/testOaiFunctionality/

I know @Louis-wr (and anyone off campus) can't see Jenkins due to a firewall but here's a screenshot:

Let me ask about re-running the tests or maybe kick off a new run myself.

Hello,
Thank you sharing the error log.
From what i can tell it cant connect to a resource at amazoneaws.com, it that case i presume this issue is not related to the code i summited

Also as this anything to do with this error ?

Extension error (sphinxcontrib.icon.icon):
Handler <function download_font_assets at 0x7fcea99cd280> for event 'builder-inited' threw an exception (exception: HTTP Error 503: Egress is over the account limit.)
make: *** [Makefile:64: html] Error 2

from https://github.com/IQSS/dataverse/runs/8014075879?check_suite_focus=true
I i interpreted this correctly an asset can not be downloaded from a Microsoft azure blob.

@pdurbin
Copy link
Member

pdurbin commented Aug 25, 2022

Hmm, I'm not too worried about the "Guides Build Status" failure because the OTHER guides job ("docs/readthedocs...") passed and built docs that look good: https://dataverse-guide--8880.org.readthedocs.build/en/8880/installation/config.html#shibaffiliationorder

By the way I opened #8937 about the first error I mentioned about harvesting. It definitely has nothing to do with this pull request.

@pdurbin pdurbin self-assigned this Sep 7, 2022
@pdurbin
Copy link
Member

pdurbin commented Sep 7, 2022

@Louis-wr I'm having a little trouble testing this PR.

Rather than setting up all of Shibboleth on a server, I'm using the method described at https://guides.dataverse.org/en/5.11.1/developers/remote-users.html

First, I add a shib auth provider:

$ cat shibAuthProvider.json
{
"id":"shib",
"factoryAlias":"shib",
"enabled":true
}

$ curl -X POST -H 'Content-type: application/json' --upload-file shibAuthProvider.json http://localhost:8080/api/admin/authenticationProviders

Then I set the "debug shib" setting, starting with "random":

$ curl http://localhost:8080/api/admin/settings/:DebugShibAccountType -X PUT -d RANDOM

However, when I go to http://localhost:8080/shib.xhtml and click "Create Account" I get this stacktrace:

[2022-09-07T13:31:43.220-0400] [Payara 5.2022.3] [WARNING] [] [javax.enterprise.web] [tid: _ThreadID=120 _ThreadName=http-thre
ad-pool::http-listener-1(2)] [timeMillis: 1662571903220] [levelValue: 900] [[
StandardWrapperValve[Faces Servlet]: Servlet.service() for servlet Faces Servlet threw exception
java.lang.NullPointerException
at java.base/java.util.HashSet.(HashSet.java:119)
at edu.harvard.iq.dataverse.authorization.providers.builtin.DataverseUserPage.init(DataverseUserPage.java:177)

Here's line 177:

mutedEmails = new HashSet<>(currentUser.getMutedEmails());

@ErykKul added this recently and might have some ideas of how to get it working with the "shib debug" method above.

@Louis-wr meanwhile, I can't push to your branch. This isn't a problem necessarily but if I were able to fix the above I'd probably just push rather than making a pull request against your pull request.

Do you want to try the "shib debug" thing above? In addition to "RANDOM" there cases like "TWO_EMAILS" and "INVALID_EMAIL". You could add a case or two having to do with affiliation.

@pdurbin
Copy link
Member

pdurbin commented Sep 7, 2022

Phew! Ok. I just created DataverseNO#44 which is how I tested this PR. Again, instead of setting up all of Shibboleth on a server, it's much easier to test using the :DebugShibAccountType settings (I just added two cases in that PR). @Louis-wr if you're willing to merge that PR, I'd appreciate it.

To test I used that PR to the "two affiliations" case like this:

curl http://localhost:8080/api/admin/settings/:DebugShibAccountType -X PUT -d TWO_AFFILIATIONS

That PR relies on setting the affiliation attribute to "ou" like this:

curl http://localhost:8080/api/admin/settings/:ShibAffiliationAttribute -X PUT -d "ou"

I left the separator undefined to get ";". Setting it should be fine.

Here are the two affiliations at login ("SNPP;Stonecutters"):

Screen Shot 2022-09-07 at 3 45 30 PM

Likewise if you just look at the account page, you can see both affiliations, separated by a semicolon:

Screen Shot 2022-09-07 at 3 45 49 PM

Now, if you say you want the last affiliation, it works:

curl -X PUT -d "lastAffiliation" http://localhost:8080/api/admin/settings/:ShibAffiliationOrder

Screen Shot 2022-09-07 at 3 47 43 PM

It also works to request the first affiliation:

curl -X PUT -d "firstAffiliation" http://localhost:8080/api/admin/settings/:ShibAffiliationOrder

Screen Shot 2022-09-07 at 3 48 14 PM

I'm satisfied enough to move this to QA but again @Louis-wr please consider merging the PR above. Thanks!

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Works fine. See comments. I'd like for DataverseNO#44 to be merged into this PR because it was useful while testing.

@pdurbin pdurbin removed their assignment Sep 7, 2022
@pdurbin
Copy link
Member

pdurbin commented Sep 7, 2022

I left the separator undefined to get ";". Setting it should be fine.

I decided I should actually test this. I set it to "," to be different than the default (";") and it correctly did NOT split SNPP;Stonecutters into two values:

curl -X PUT -d "," http://localhost:8080/api/admin/settings/:ShibAffiliationSeparator

Screen Shot 2022-09-07 at 4 09 39 PM

@pdurbin
Copy link
Member

pdurbin commented Sep 9, 2022

@ErykKul added this recently and might have some ideas of how to get it working with the "shib debug" method above.

I've been talking with @ErykKul and he came up with a fix in PR #8969. I haven't played with that code. It wasn't required for me to test this PR. Until a fix like that is merged, you just have to be conscious that you might get errors from Shib with the debug method (and maybe prod Shib?). The work around is to log out and back in so that info about your user comes from the database. It's confusing but out of scope for this PR.

I ran the API tests and they passed. I think we're good so I'm merging this.

@pdurbin pdurbin merged commit c7b8b82 into IQSS:develop Sep 9, 2022
@pdurbin pdurbin added this to the 5.12 milestone Sep 10, 2022
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.

shib-affiliation

4 participants