flipboxfactory / saml-sp

SAML Service Provider (SP) Plugin for Craft CMS
https://saml-sp.flipboxfactory.com/
Other
19 stars 5 forks source link

Craft 3.4 and headlessMode compatibility #53

Closed rocknrolaf closed 4 years ago

rocknrolaf commented 4 years ago

Hi there,

first of all thank you for the plugin! Currently we have an issue with Craft 3.4 and the attribute mapping of custom fields for users. The default fields like email, firstname and lastname get synced correctly but custom fields won't. I guess this could be due to the new Craft version only accepting "setFieldValue" now (https://craftcms.com/guides/updating-plugins-for-craft-34#setting-custom-field-values). Possibly this could be solved in the assignProperty function of the User class: https://github.com/flipboxfactory/saml-sp/blob/master/src/services/login/User.php#L236

Another issue we have is with "headlessMode" enabled in the general config for Craft the redirect to the ADFS won't happen because the default server response in json format isn't evaluated as html. This response happens within here: https://github.com/flipboxfactory/saml-core/blob/master/src/containers/Saml2Container.php#L117 Maybe this would need to be an html format response.

Thank you.

dsmrt commented 4 years ago

Hello @rocknrolaf,

Thanks for checking out the plugin! And thanks for being so clear on what your issues are ... this is very helpful.

The first issues with v3.4 makes perfect sense. Thanks for finding that!

The second issue seem to make sense. Correct me if I am wrong here: You have the Craft install set to headlessMode and sense I'm not explicitly setting the response to html it's showing as json.

Both of these seem like easy patches. I'll get a patch going in the next couple days.

rocknrolaf commented 4 years ago

@dsmrt yes thats right! i see the html markup output as a string in the browser. currently i use this workaround:

        if (Craft::$app->getRequest()->getIsSiteRequest() && preg_match('/^sso\/login\/request/', Craft::$app->request->getPathInfo())) {
            Event::on(
                Response::class,
                Response::EVENT_BEFORE_SEND,
                function (Event $event) {
                    /** @var \craft\web\Response $response */
                    $response = $event->sender;
                    $response->format = Response::FORMAT_HTML;
                }
            );
        }

which seems to work. so just setting the response format should do the trick. thanks for the quick response!

dsmrt commented 4 years ago

I didn't mean to close that.

dsmrt commented 4 years ago

Ok ... I think I have both of these going. Update saml-core and saml-sp to 2.1.3 and see how that goes. Let me know if you run into any issues here or with anything else.

rocknrolaf commented 4 years ago

@dsmrt unfortunately there seems to be an error for the user attributes which are not custom fields. maybe there needs to be a separate handling for default user attributes and custom fields. see following error:

2020-03-05 01:25:08 [-][-][fda04c1cebbc545f9408835e25242265][error][yii\base\UnknownPropertyException] yii\base\UnknownPropertyException: Setting unknown property: craft\behaviors\CustomFieldBehavior::email in /var/www/html/vendor/yiisoft/yii2/base/BaseObject.php:163
Stack trace:
#0 /var/www/html/storage/runtime/compiled_classes/CustomFieldBehavior.php(412): yii\base\BaseObject->__set('email', '....@fo...')
#1 /var/www/html/vendor/craftcms/cms/src/base/Element.php(2168): craft\behaviors\CustomFieldBehavior->__set('email', '....@fo...')
#2 /var/www/html/vendor/flipboxfactory/saml-sp/src/services/login/User.php(236): craft\base\Element->setFieldValue('email', '.....@fo...')
dsmrt commented 4 years ago

Ok. I didn’t see this with my testing on a fresh install. I’ll check in a bit.

dsmrt commented 4 years ago

Alright ... Looks like I was getting some false positives with my testing yesterday. I have that all fixed up on 2.1.4. If you could give that a test and let me know how that goes, I greatly appreciate it!

rocknrolaf commented 4 years ago

@dsmrt yes it works fine! thank you!