search

mattbrailsford / umbraco-authu

An Umbraco plugin to add an OAuth API endpoint to allow authenticated Members/Users via OAuth
MIT License
71 stars 22 forks source link

Adding a refreshtoken does not properly delete old ones #52

Open somoreingold opened 4 years ago

somoreingold commented 4 years ago

First off, thanks for this extremely elegant library!

I found an issue with the included UmbracoDbOAuthRefreshTokenStore. The SQL delete statement below does not take into account optional fields (for example, DeviceId) that may therefore be null parameter values:

https://github.com/mattbrailsford/umbraco-authu/blob/407ce4185c82c76466902ef095fa0aa3c205b038/src/Our.Umbraco.AuthU/Data/UmbracoDbOAuthRefreshTokenStore.cs#L16

In SQL you cannot use the = sign to compare against NULL (you have to use ' ... is NULL'. ). Therefore, if DeviceId was made optional/null coming in, the SQL statement would never find the appropriate row to delete since the statement would evaluate to:

... AND [DeviceId] = NULL

This results in a table that keeps growing. To fix, you could either detect if token.DeviceId is null then simply not add that part of the SQL string. Or I guess you could do something with SqlContext to build the query...not sure what's better. What do you think?

mattbrailsford commented 4 years ago

Good spot. I guess it would be better to make it an ‘is NULL’ statement as otherwise removing the parameters could result in some being deleted if for some reason you had a mixture where some have a DeviceId but some don’t.

somoreingold commented 4 years ago

Agreed, that definitely seems like the safer way to do it. Thank you!

I have an unrelated question (not a bug) concerning the symmetric key--would you prefer I create another issue, or do you prefer another mode of communication for non-bug questions about AuthU?

ghost commented 3 years ago

Ah, I think this explains why I found 250k+ records in the database table. Could that also explain the very, very slow authentication call resulting in the message "Execution Timeout Expired. The timeout period elapsed prior to completion of the operation or the server is not responding."?

Other than that the package served us perfectly in the past year, even after upgrading from V7 to V8, so a big H5YR to you @mattbrailsford