dataverse icon indicating copy to clipboard operation
dataverse copied to clipboard

Add Shib attribute characterset conversion to getValueFromAssertion

Open PaulBoon opened this issue 2 years ago • 7 comments

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:

PaulBoon avatar Mar 22 '23 14:03 PaulBoon

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.

PaulBoon avatar Mar 30 '23 08:03 PaulBoon

@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?

PaulBoon avatar Jan 16 '24 16:01 PaulBoon

@cmbz @scolapasta how do you feel about getting this fix in?

pdurbin avatar Jan 16 '24 16:01 pdurbin

@pdurbin I have no objection. @scolapasta?

cmbz avatar Jan 16 '24 19:01 cmbz

@PaulBoon I'm afraid there are merge conflicts. Do you mind resolving them?

pdurbin avatar Sep 30 '24 18:09 pdurbin

@pdurbin Should be OK now. BTW the added code is just the same as in getRequiredValueFromAssertion.

PaulBoon avatar Oct 01 '24 11:10 PaulBoon

@PaulBoon Can you please add testing steps for this ticket. Thank you!

ofahimIQSS avatar Oct 21 '24 13:10 ofahimIQSS

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

PaulBoon avatar Oct 30 '24 12:10 PaulBoon

@ofahimIQSS I was hoping someone involved with the original issue or code for the getRequiredValueFromAssertion could test it.

PaulBoon avatar Oct 30 '24 12:10 PaulBoon

@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?

ofahimIQSS avatar Nov 01 '24 19:11 ofahimIQSS

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:

Screenshot 2024-11-01 at 4 17 17 PM

Screenshot 2024-11-01 at 4 17 24 PM

Here's a commit I pushed for the above to a test branch: 28b6779489

Again, I'm not sure how to reproduce the bug. 🤷

pdurbin avatar Nov 01 '24 20:11 pdurbin

@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.

PaulBoon avatar Nov 05 '24 15:11 PaulBoon

@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 avatar Nov 05 '24 15:11 PaulBoon

@PaulBoon probably. The whole file is UTF-8:

% file ~/Downloads/ShibUtil.java 
/Users/PDurbin/Downloads/ShibUtil.java: Java source, Unicode text, UTF-8 text

pdurbin avatar Nov 05 '24 19:11 pdurbin

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 avatar Nov 06 '24 09:11 PaulBoon

@PaulBoon good idea. I tried this in https://github.com/PaulBoon/dataverse/pull/9/commits/368bcc9ddced52d0b6b07f2a571fbd10ca1177ec but "Universität" still looks fine.

Screenshot 2024-11-06 at 10 47 59 AM

Screenshot 2024-11-06 at 10 52 45 AM

I can't push to your branch, by the way. That's why I created https://github.com/PaulBoon/dataverse/pull/9

pdurbin avatar Nov 06 '24 15:11 pdurbin

No issues found during Regression. Merging PR.

https://github.com/user-attachments/assets/4c12b148-a584-4383-bea1-9023fc467c84

ofahimIQSS avatar Nov 06 '24 19:11 ofahimIQSS