Skip to content

Add Shib attribute characterset conversion to getValueFromAssertion#9461

Merged
ofahimIQSS merged 4 commits intoIQSS:developfrom
PaulBoon:8573-Shib-attribute-character-set-conversion-for-affiliation
Nov 6, 2024
Merged

Add Shib attribute characterset conversion to getValueFromAssertion#9461
ofahimIQSS merged 4 commits intoIQSS:developfrom
PaulBoon:8573-Shib-attribute-character-set-conversion-for-affiliation

Conversation

@PaulBoon
Copy link
Contributor

@PaulBoon PaulBoon commented Mar 22, 2023

What this PR does / why we need it:
Adds optional Shib attribute characterset conversion to getValueFromAssertion

Which issue(s) this PR closes:

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:

@pdurbin pdurbin added Feature: Account & User Info Size: 3 A percentage of a sprint. 2.1 hours. labels Mar 27, 2023
@PaulBoon PaulBoon marked this pull request as ready for review March 30, 2023 07:53
@PaulBoon
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@pdurbin
Copy link
Member

pdurbin commented Jan 16, 2024

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

@cmbz
Copy link

cmbz commented Jan 16, 2024

@pdurbin I have no objection. @scolapasta?

@cmbz cmbz added the FY25 Sprint 7 FY25 Sprint 7 (2024-09-25 - 2024-10-09) label Sep 30, 2024
@pdurbin
Copy link
Member

pdurbin commented Sep 30, 2024

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

@PaulBoon
Copy link
Contributor Author

PaulBoon commented Oct 1, 2024

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

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.

Thanks. I didn't test this but the change makes sense. Approved.

@pdurbin pdurbin changed the title 8573 Add Shib attribute characterset conversion to getValueFromAssertion Add Shib attribute characterset conversion to getValueFromAssertion Oct 1, 2024
@cmbz cmbz added the FY25 Sprint 8 FY25 Sprint 8 (2024-10-09 - 2024-10-23) label Oct 9, 2024
@pdurbin pdurbin added the Type: Feature a feature request label Oct 9, 2024
@ofahimIQSS
Copy link
Contributor

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

@cmbz cmbz added the FY25 Sprint 9 FY25 Sprint 9 (2024-10-23 - 2024-11-06) label Oct 23, 2024
@PaulBoon
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@ofahimIQSS
Copy link
Contributor

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

pdurbin added a commit that referenced this pull request Nov 1, 2024
@pdurbin
Copy link
Member

pdurbin commented Nov 1, 2024

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: 28b6779

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

@PaulBoon
Copy link
Contributor Author

PaulBoon commented Nov 5, 2024

@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
Copy link
Contributor Author

PaulBoon commented Nov 5, 2024

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

@pdurbin
Copy link
Member

pdurbin commented Nov 5, 2024

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

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

@PaulBoon
Copy link
Contributor Author

PaulBoon commented Nov 6, 2024

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);

@pdurbin
Copy link
Member

pdurbin commented Nov 6, 2024

@PaulBoon good idea. I tried this in PaulBoon@368bcc9 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 PaulBoon#9

@ofahimIQSS ofahimIQSS self-assigned this Nov 6, 2024
@ofahimIQSS
Copy link
Contributor

No issues found during Regression. Merging PR.

testing.shib.mov

@ofahimIQSS ofahimIQSS merged commit b28812b into IQSS:develop Nov 6, 2024
@ofahimIQSS ofahimIQSS removed their assignment Nov 6, 2024
@pdurbin pdurbin added this to the 6.5 milestone Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature: Account & User Info FY25 Sprint 7 FY25 Sprint 7 (2024-09-25 - 2024-10-09) FY25 Sprint 8 FY25 Sprint 8 (2024-10-09 - 2024-10-23) FY25 Sprint 9 FY25 Sprint 9 (2024-10-23 - 2024-11-06) Size: 3 A percentage of a sprint. 2.1 hours. Type: Feature a feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ShibAttributeCharacterSetConversionEnabled has no influence on affiliation

4 participants