Add Shib attribute characterset conversion to getValueFromAssertion
What this PR does / why we need it: Adds optional Shib attribute characterset conversion to getValueFromAssertion
Which issue(s) this PR closes:
- Closes #8573
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation:
This has not been tested. Also we might want to throw an exception and have the init() return as is done for the required fields.
@pdurbin We are running this in production for some time now, whithout any problems. Could this small change be merged in the next version please?
@cmbz @scolapasta how do you feel about getting this fix in?
@pdurbin I have no objection. @scolapasta?
@PaulBoon I'm afraid there are merge conflicts. Do you mind resolving them?
@pdurbin Should be OK now. BTW the added code is just the same as in getRequiredValueFromAssertion.
@PaulBoon Can you please add testing steps for this ticket. Thank you!
Testing is difficult because we don't have a testIdp where we can fake an affiliation. But now that I had a peek at the code change, I do see it is wrong; it is now not mapping to StandardCharsets.UTF_8
@ofahimIQSS I was hoping someone involved with the original issue or code for the getRequiredValueFromAssertion could test it.
@PaulBoon Hey Paul - does your institution show up on the list of Institutions and when you login, is your information garbled or can you make it garbled? This is on https://demo.dataverse.org/ - Can you reproduce the bug there and paste a screenshot?
I'm having trouble reproducing the bug. In the issue (#8573) the suggestion was to use "Universität", but it seems to work fine, at least when I use :DebugShibAccountType as described at https://guides.dataverse.org/en/6.4/developers/remote-users.html#shibboleth-and-oauth
Maybe we need a real Shibboleth setup to test with? And access to an account that has "Universität" or similar to test with?
Anyway, here are screenshots from my test:
Here's a commit I pushed for the above to a test branch: 28b6779489
Again, I'm not sure how to reproduce the bug. 🤷
@ofahimIQSS Sorry for my late reply, I tend to ignore my mailbox regulary. About the login, the dropdown with institutes looks fine, but I can not find my institute (DANS), also not KNAW.
@pdurbin That you could not reproduce the problem might have to do with the input, I mean that string was probably all ready in UTF-8 when you entered it?
@PaulBoon probably. The whole file is UTF-8:
% file ~/Downloads/ShibUtil.java
/Users/PDurbin/Downloads/ShibUtil.java: Java source, Unicode text, UTF-8 text
Unless I made a copy error, it should work because I assume the existing code in getRequiredValueFromAssertion is correct, which I copied into getValueFromAssertion.
So, testing is gold, but we could go for code inspection (silver or bronze?) instead.
@pdurbin You could get the test working with the obvious reverse; new String("SNPP;Universität".getBytes(StandardCharsets.UTF_8), StandardCharsets.ISO_8859_1);
@PaulBoon good idea. I tried this in https://github.com/PaulBoon/dataverse/pull/9/commits/368bcc9ddced52d0b6b07f2a571fbd10ca1177ec but "Universität" still looks fine.
I can't push to your branch, by the way. That's why I created https://github.com/PaulBoon/dataverse/pull/9
No issues found during Regression. Merging PR.
https://github.com/user-attachments/assets/4c12b148-a584-4383-bea1-9023fc467c84