catalyst / moodle-auth_saml2

SAML done 100% in Moodle, fast, simple, secure
https://moodle.org/plugins/auth_saml2
70 stars 132 forks source link

After Update (Moodle 4.1) warning message upon login: "Your idnumber wasn't updated" #753

Open peta3000 opened 1 year ago

peta3000 commented 1 year ago

What happened?

Actual Behavior: After upgrading a Moodle instance from 3.9 to 4.1.1 (Build: 20230116), upon login via sso (saml2) users are greeted with the warning message: "Your idnumber wasn't updated"

--> besides this, the SSO login via saml2 works fine, though

Expected behavior: Message is not shown.

Version used: 2022111700 --> updated to commit 1992e3d1c8 on branch MOODLE_39_STABLE: https://github.com/catalyst/moodle-auth_saml2/blob/1992e3d1c8452fb5657921a63dd36e3c2e6d8e27/version.php

Plugin-Settings: (selection of some relevant mappings) auth_saml2 | idpattr = idnumber auth_saml2 | mdlattr = ID number auth_saml2 | field_updatelocal_idnumber = On every login

Related issue: A related issue (solved) with similar warning message but usage of user email-address for mapping (idpattr, mdlattr):

728

Although this behavior was fixed for usage of email as mapping between idp and moodle, I was wandering whether this fix hasn't included other mapping-variants and whether this warning message could be disabled for all mapping-fields-variants?

Thanks a lot and best regards

peta

peta3000 commented 1 year ago

Dear @danmarsden I was wondering if there is any chance that a patch to resolve the documented issue might be included in one of the next releases of the plugin? Since it seems to be closely related to the already resolved issue #728 fixed by #730 which in turn was caused by: #715

Many thanks and have a lovely day

peta

peta3000 commented 1 year ago

Dear @danmarsden I totally understand that there is a huge pile of open issues demanding the dev's attention. I would merely like to inquire, whether there are any chances that a patch for the reported issue wold be considered to be included in the future releases?

Many thanks and best regards

peta

danmarsden commented 1 year ago

Hi @peta3000 - our ability to provide development support "for free" is pretty limited - we tend to fix bugs when we run into them ourselves, or when our clients hit the issues and we need to fix them.

If you know someone with development capabilities, feel free to submit a pull request and we'll take a look, otherwise feel free to reach out privately if you would like commercial level support and would like to fund improvements or bug fixes.

thanks!

peta3000 commented 1 year ago

Hi @danmarsden Thanks a lot for taking the time to reply and to explain your reasons!

Cheers

peta

sarahjcotton commented 9 months ago

Hi @peta3000 @danmarsden - We have also stumbled across this issue, and having looked at the various related issues, it feels to me as though the plugin is doing the right thing by throwing an error, however it might be better to stop that configuration happening in the first place.

I would like to propose updating the settings so that whatever is selected in mdlattr, the equivalent mapping is set to 'Update on creation' and that form element become locked/disabled. That way we shouldn't run into the scenario where the error is thrown, regardless of which value is selected for mdlattr (I noticed a similar case where the same thing happened when email was selected).

Although the email issue was fixed in PR 730, the fact that the 'Update on login' can still be set feels a bit misleading, hence the suggestion of enhancing the settings.

I hope that makes sense; thoughts welcome. Sarah

danmarsden commented 9 months ago

Thanks Sarah - I'd add a +1 - makes sense, but I have a vague recollection that this uses the settings api's - not mforms, and I don't think the settings api has a disabledif style handler so it might need a bit of work to implement. (didn't pull up the code though to verify)

sarahjcotton commented 9 months ago

Yes you're right... I spent a bit of time yesterday trying to force the current settings page to do what I wanted but kept hitting dead ends after getting part way there.

Edit: I've managed to get some JS to load on the settings page, so could manipulate the form fields that way if that's an acceptable approach?