0

So I am writing a custom membership provider for Sitecore and my strategy was to download the source code to the SQLMembershipProvider (which sitecore already uses) and just add my custom logic to it.

One of the core functionalities we wanted was the ability to store a password history and I was working in the ChangePassword function to start.

The first few times I tried my code I had no issue, but now even after computer restarts, attempting to disconnect all users from the database, and refactoring my code so that it only uses a single SQLCommand and one connection I cannot get rid of this error.

I'm including the relevant code from the change password function (I demarked the custom code I added with //my code - start/end). Any ideas?

try { SqlConnectionHolder recentpw_holder = null; SqlDataReader reader = null; SqlCommand tempcmd = null; try { recentpw_holder = SqlConnectionHelper.GetConnection(_sqlConnectionString, true); CheckSchemaVersion(recentpw_holder.Connection); //my code - Start tempcmd = new SqlCommand("SELECT * FROM dbo.PasswordHistory WHERE username='" + username + "' ORDER BY date_created", recentpw_holder.Connection); using (reader = tempcmd.ExecuteReader()) { var passwordlist = new List<string>(); while (reader.Read()) { //get the list of passwords for (int i = 0; i < reader.FieldCount; i++) { passwordlist.Add(reader.GetValue(i).ToString()); } } //if there are more than 10 passwords stored in the database remove the oldest one if (passwordlist.Count > 10) { tempcmd.CommandText = "DELETE * FROM dbo.PasswordHistory" + "WHERE username='" + username + "'" + "AND password='" + passwordlist.First() + "'"; tempcmd.ExecuteNonQuery(); passwordlist.RemoveAt(0); } //check and see if that password has already been used, if so throw an error foreach (var p in passwordlist) { if (p == pass) { throw new ArgumentException("Unable to create password. Password does not meet the history requirements of the domain."); } } } //my code - end tempcmd.CommandText = "dbo.aspnet_Membership_SetPassword"; tempcmd.CommandTimeout = CommandTimeout; tempcmd.CommandType = CommandType.StoredProcedure; tempcmd.Parameters.Add(CreateInputParam("@ApplicationName", SqlDbType.NVarChar, ApplicationName)); tempcmd.Parameters.Add(CreateInputParam("@UserName", SqlDbType.NVarChar, username)); tempcmd.Parameters.Add(CreateInputParam("@NewPassword", SqlDbType.NVarChar, pass)); tempcmd.Parameters.Add(CreateInputParam("@PasswordSalt", SqlDbType.NVarChar, salt)); tempcmd.Parameters.Add(CreateInputParam("@PasswordFormat", SqlDbType.Int, passwordFormat)); tempcmd.Parameters.Add(CreateInputParam("@CurrentTimeUtc", SqlDbType.DateTime, DateTime.UtcNow)); SqlParameter para = new SqlParameter("@ReturnValue", SqlDbType.Int); para.Direction = ParameterDirection.ReturnValue; tempcmd.Parameters.Add(para); tempcmd.ExecuteNonQuery(); //my code pt 2 - start tempcmd.Parameters.Clear(); tempcmd.CommandType = CommandType.Text; tempcmd.CommandText = "INSERT INTO dbo.PasswordHistory " + "(username, password, date_created) " + "VALUES ('" + username + "', '" + pass + "', '" + DateTime.UtcNow + "')"; tempcmd.ExecuteNonQuery(); //my code pt 2 - end status = ( ( para.Value != null ) ? ( ( int )para.Value ) : -1 ); if ( status != 0 ) { string errText = GetExceptionText( status ); if ( IsStatusDueToBadPassword( status ) ) { throw new MembershipPasswordException( errText ); } else { throw new ProviderException( errText ); } } return true; } finally { if (reader != null) { reader.Close(); reader = null; } if( recentpw_holder != null ) { recentpw_holder.Close(); recentpw_holder = null; } } } catch { throw; }
Improve this question
2
  • 1
    I've found it's best not to reuse SqlCommand objects. I prefer to do using (var command = new SqlCommand()) { ... } to avoid nesting commands. It looks like you have nested commands. Your sql to delete from PasswordHistory is nested. You don't mention where the error happens. However, I suspect the error happens on that command. If you move it outside of using (reader = tempcmd.ExecuteReader()) { ... } the error should go away. Also, I suggest never having SqlReader variables outside a using block. So remove SqlReader reader = null; and declare it in the using block. Commented Sep 24, 2015 at 17:41
  • 3
    And Please parameterize that SQL, especially when dealing with user tables, the 1st 2 SQL statements in that leave a massive security hole in your code. xkcd.com/327 Commented Sep 24, 2015 at 18:23

1 Answer 1

Reset to default
0

You need to dispose of that reader once you no longer need it. Try this:

 using (reader = tempcmd.ExecuteReader()) { var passwordlist = new List<string>(); while (reader.Read()) { //get the list of passwords for (int i = 0; i < reader.FieldCount; i++) { passwordlist.Add(reader.GetValue(i).ToString()); } } } //if there are more than 10 passwords stored in the database remove the oldest one if (passwordlist.Count > 10) { tempcmd.CommandText = "DELETE * FROM dbo.PasswordHistory" + "WHERE username='" + username + "'" + "AND password='" + passwordlist.First() + "'"; tempcmd.ExecuteNonQuery(); passwordlist.RemoveAt(0); } //check and see if that password has already been used, if so throw an error foreach (var p in passwordlist) { if (p == pass) { throw new ArgumentException("Unable to create password. Password does not meet the history requirements of the domain."); } }
Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.