flipboxfactory / saml-sp

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

EntityID getting set to base URL (project.yaml) #29

Closed SwiseWeb closed 5 years ago

SwiseWeb commented 5 years ago

Hello! I'm working through configuring this plugin locally and hit a strange snag. Here are the steps I've taken:

  1. Configured My Provider with entityID set to the default value of http://cms.mydomain.test:8080 (local URL)
  2. Configured the remote IDP
  3. Attempt to hit login URL and receive Trying to get property 'keychain' of non-object similar to #26

Upon hitting that issue, I looked through the related closed issue and confirmed that my local service provider was set up properly. I then edited the LoginController.php file and dropped in the following before the authnRequest:

return $this->asJson(array(
      "entitySettings" => Saml::getInstance()->getSettings(),
      "entity" => Saml::getInstance()->getProvider()->findByEntityId(Saml::getInstance()->getSettings()->getEntityId())->one()
));

Resulting in:

 {"entityID": {
  "enableCpLoginButtons":true,
  "enableUsers":true,
  "signAuthnRequest":true,
  "wantsSignedAssertions":true,
  "mergeLocalUsers":true,
  "createUser":true,
  "autoCreateGroups":true,
  "syncGroups":true,
  "groupAttributeNames":["groups"],
  "defaultGroupAssignments":[],
  "responseAttributeMap":{"http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress":"email",
  "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname":"firstName",
  "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname":"lastName",
  "email":"email",
  "firstName":"firstName",
  "lastName":"lastName"},
  "relayStateOverrideParam":"RelayState",
  "loginRequestEndpoint":"/sso/login/request",
  "logoutRequestEndpoint":"/sso/logout/request",
  "entityId":"myDomain:8080/"
},
"entity":null
}

What I quickly noticed is that the entityID which was getting returned is equal to the frontend application URL and not the Craft CMS URL. That app URL is set as the BaseURL which led me to the AbstractSettings where the entityID defaults to the BaseURL if there is no entityID. If I update the BaseURL to http://cms.myDomain.test:8080, then everything works as expected since the BaseURL is now correct and it has no problem finding the entity above.

So I guess my question is how is that entityID supposed to get set in the plugin and what would cause it to be null and therefore default to the BaseURL? I've checked all plugin settings locations for the entityID and they're all set properly.

SwiseWeb commented 5 years ago

Update: After going back to the plugin settings page and hitting Save, the entityID is now displaying the proper value. Not really sure how that value got unset/cleared to begin with, but it's working as expected now ¯_(ツ)_/¯

Here's what I discovered during the debug process - if you go to the Settings page and clear out the entityID and then hit Save, the entityID will now default to the BaseURL as expected based on the code. However, if you go back to the plugin settings page, it will display the default value in the box rather than being empty even though the entityID is technically blank. Maybe it makes sense to either have a requirement around that field or at least show that the entityID is indeed blank rather than showing a value in the settings field.

dsmrt commented 5 years ago

Hello , Thanks for trying out the plugin!

I think there are 2 things going on here....

1. Setting Entity ID Manually

I think I'm following correctly. Sounds like you are saving the entity id as something custom, correct? Usually, I let the plugin set the entityId setting dynamically based on the base url. I can see there being cases where you have to overwrite that but you really shouldn't have to. In plugins table you can try deleting the key pair value ("entityId":"https://saved-entity-id") and see if that problem persists. Double check the config/saml-sp.php as well to make sure you're not setting it there.

2. Create/Re-Create My Provider

Once you have verified that the custom/manually set entityId is not set (see above), make sure you re-save "My Provider" so that the metadata reflects everything correctly. This must be saved and up-to-date when that is changed.

SwiseWeb commented 5 years ago

@dsmrt Thanks for the reply! I did a little more work on this and the problem I was having seems to be coming down to Saml::getInstance()->getSettings()->getEntityId() returning different values if the entityId in the plugin settings is blank. Here's what I just did:

  1. Went to /admin/saml-sp/settings and cleared out the entityId value and hit Save.
  2. Checked the project.yaml file and noted that entityId field is indeed blank
  3. Reloaded the Settings page and saw the default value populated into the Settings field even though it should be blank
  4. Went over to the login page and saw the same keychain error and when adding the debug code above, noted that the entityId was getting set to the BASE_URL

After that, I checked the value of Saml::getInstance()->getSettings()->getEntityId() on both the plugin settings template page and the login page and turns out those two values are different when the entityId field on the settings page has been blanked out on the most recent save.

Are you able to test the above and see if you get the same result?

dsmrt commented 5 years ago

Ok, got it! This helps a ton. The problem here is with the project.yaml. This plugin doesn't yet fully support the project.yaml. I'm going to start working on a patch but it looks like you can remove, either, the entire setting block, OR the entityId line. See the screenshot below. This will reset things properly.

Screen Shot 2019-08-16 at 4 13 16 PM

dsmrt commented 5 years ago

I think the best way to patch this issue is to change the entityId setting field to allow environmental variables. That way you can change that value based on environment. With this, you will still need to create "My Provider" on each environment.

How does that sound?

SwiseWeb commented 5 years ago

Ahhhhh, that definitely makes sense. Yes, being able to set that via either a config variable or an environment variable sounds great. Otherwise it seems like I may run into problems where the entityId is set incorrectly via the project.yaml file since there's no environment awareness there. That solution would also play nicely with the automation that I have set up for this site specifically.

Thanks for working through this!

dsmrt commented 5 years ago

Added in https://github.com/flipboxfactory/saml-sp/releases/tag/2.0.0