search

umbraco / Umbraco.AuthorizedServices

Umbraco Authorized Services is an open-source package designed to reduce the effort needed to integrate third party services that require authentication and authorization via an OAuth flow.
MIT License
14 stars 7 forks source link

`TokenEncryptionKey` is described as optional, but encoding fails if not specified #13

Closed abjerner closed 1 year ago

abjerner commented 1 year ago

According to the README.md the TokenEncryptionKey is optional, but trying to authenticate via the package, encoding fails as a secret key is required:

https://github.com/umbraco/Umbraco.AuthorizedServices/blob/main/src/Umbraco.AuthorizedServices/Services/Implement/AesSecretEncryptor.cs#L17

So the question is - should the README be updated? Or should the behavior be updated to align with the README?

AndyButland commented 1 year ago

My thinking is we should allow this to be optional, but make sure not to fail if it's not provided. I've done that with the linked PR. If you have time for a look and have any thoughts, would be appreciated.

abjerner commented 1 year ago

It takes care of the barrier for using the package, which I think is good, but I'm not super thrilled about storing unencoded tokens. Would it be an idea to use Umbraco:CMS:Global:Id as the secret in case a TokenEncryptionKey isn't provided?

And perhaps show an warning/alert in the backoffice when the "fallback" is used?

AndyButland commented 1 year ago

Makes sense, have updated to do that.

ronaldbarendse commented 1 year ago

The CMS already configures ASP.NET Core Data Protection, so it probably makes much more sense to make DataProtectionSecretEncryptor the default:

FYI Data Protection needs to be correctly configured anyway, because other parts of the CMS otherwise won't work (like Html.BeginUmbracoForm() that uses an encrypted route string). The default configuration should also be fine for most users, so this is a plug-and-play provider.

Also note that DataProtectionSecretEncrytor contains a spelling error...

AndyButland commented 1 year ago

Thanks. We did have this as the default at one point, but made it the optional implementation... I'm trying to think why. Possibly related to a concern about if you were load balancing, where an encrypted value from one node wouldn't be accessile on another? But I hadn't realised the CMS was configuring it already.

Changing the default would cause a couple of issues:

We are still in 0.X experimental version stage, but because of the above I think we should leave this for now and leave it to the implementer to swap if they want.

Spelling mistake has been fixed in the PR for this issue, and I've also removed from the docs the noted need to enable data protection.

AndyButland commented 1 year ago

Closing this now as release 0.2.0 will contain this fix and also a switch to using DataProtectionSecretEncryptor as the default: