1

Closed

CreateOrUpdateOAuthAccount inserts without upper, updates using upper comparison.

description

CreateOrUpdateOAuthAccount in WebMatrix.WebData.SimpleMembershipProvider contains the following code:
if (oldUserId == -1)
                {
                    // account doesn't exist. create a new one.
                    int insert = db.Execute(@"INSERT INTO [" + OAuthMembershipTableName + "] (Provider, ProviderUserId, UserId) VALUES (@0, @1, @2)", provider, providerUserId, userId);
                    if (insert != 1)
                    {
                        throw new MembershipCreateUserException(MembershipCreateStatus.ProviderError);
                    }
                }
                else
                {
                    // account already exist. update it
                    int insert = db.Execute(@"UPDATE [" + OAuthMembershipTableName + "] SET UserId = @2 WHERE UPPER(Provider)=@0 AND UPPER(ProviderUserId)=@1", provider, providerUserId, userId);
                    if (insert != 1)
                    {
                        throw new MembershipCreateUserException(MembershipCreateStatus.ProviderError);
                    }
                }
Not only will the UPPER() statements in the second query provide poor query performance forcing full table seeks, the comparison in the update differs from the insertion.

Given that at least google's provider userid contains both lower case and upper case letters and is stored such in the database, UPPER() must be a mistake.
Closed Aug 19, 2013 at 10:57 PM by eilonlipton
Hi TheCarlR,

Thank you for reporting this issue. The "Simple Membership" library that was introduced with ASP.NET Web Pages was intended for relatively small-scale systems, so ease of use was the primary goal. With regard to functional issues in Simple Membership, we are focusing our efforts on the highest rated and most severe issues. We did fix some normalization/collation issues in the fix for these two issues:
https://aspnetwebstack.codeplex.com/workitem/480
https://aspnetwebstack.codeplex.com/workitem/714

And you can see the fix here:
https://aspnetwebstack.codeplex.com/SourceControl/changeset/d8d15ae5e4f49bcc4feea75193a351e4b29cce32

We are now working on an updated system called ASP.NET Identity that is meant to scale to any size site, and with additional advanced scenarios such as customizing the storage, schema, and other aspects of the system.

You can read a bit more on the new ASP.NET Identity system on these two sites:
http://blogs.msdn.com/b/webdev/archive/2013/06/27/introducing-asp-net-identity-membership-system-for-asp-net-applications.aspx
http://www.asp.net/vnext/overview/latest/release-notes#TOC8

Please check out the new systems and let us know if you have any feedback on them.

Thanks,
Eilon

comments

TheCarlR wrote Aug 2, 2013 at 11:08 PM

DeleteOAuthAccount contains the same issue.

GetUserIdFromOAuth uses UPPER but adds .ToUpperInvariant() to the parameters.