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

#1 - Allow multiple idp's with the same metadatalink #2

Merged
merged 6 commits into from
Feb 16, 2024
Merged

#1 - Allow multiple idp's with the same metadatalink #2

merged 6 commits into from
Feb 16, 2024

Conversation

Anders-Oerbaek-Pedersen
Copy link

Fix for #1

Notable changes:

  • Changed use of internal array key by from non unique database field entityid to unique database field id

@Anders-Oerbaek-Pedersen
#1 - Changed keyby for idp's to use internal id instead of entryid
6b43f65 
Entryid is not unique. Could result in same idp's entryid would only show last.
@Anders-Oerbaek-Pedersen
#1 - Changed plugin version
295edcc
frederikmillingpytlick
frederikmillingpytlick requested changes Feb 2, 2024
View reviewed changes
classes/auth.php Outdated Show resolved Hide resolved
@Anders-Oerbaek-Pedersen Anders-Oerbaek-Pedersen linked an issue Feb 2, 2024 that may be closed by this pull request
Allow multiple idp's with the same metadatalink #1
Closed
@Anders-Oerbaek-Pedersen
#1 - Removed line accidentially left after initial changes
2b5ccec
@Anders-Oerbaek-Pedersen
#1 - Changed use of entityid as array key to id
ccf0f43 
Oversight on inital changes, resulting in rows not being deleted, as expected when updating idp's
@Logiar
Copy link
Collaborator

Logiar commented Feb 13, 2024 •
edited
Loading

When saving on /admin/settings.php?section=authsettingsaml2 it does not map them to the old rows, causing new rows to be inserted and old ones to get wiped. This causes a reset to default for settings stored in auth_saml2_idps table. classes/admin/setting_idpmetadata.php:158 is inconsistent with classes/admin/setting_idpmetadata.php:93 and database id can't be used for mapping here.

@Anders-Oerbaek-Pedersen
Copy link
Author

Anders-Oerbaek-Pedersen commented Feb 15, 2024 •
edited
Loading

Mapping should be fixed now.
If the override name is changed, mapping will not match with the previous, causing the old row to be removed and a new row to be inserted.
This is caused by having nothing unique to map to, when allowing two identical IdP metadatalinks and IdP entries being declared as one text input.

@Anders-Oerbaek-Pedersen
Copy link
Author

Tested via show and tell with: JRM

@Anders-Oerbaek-Pedersen Anders-Oerbaek-Pedersen merged commit 9125e53 into MOODLE_39_STABLE-796 Feb 16, 2024
@Anders-Oerbaek-Pedersen Anders-Oerbaek-Pedersen deleted the anp_GH-1_39 branch February 16, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sampraxis sampraxis sampraxis approved these changes

@frederikmillingpytlick frederikmillingpytlick frederikmillingpytlick approved these changes

Assignees

@Anders-Oerbaek-Pedersen Anders-Oerbaek-Pedersen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow multiple idp's with the same metadatalink
4 participants
@Anders-Oerbaek-Pedersen @Logiar @sampraxis @frederikmillingpytlick