Remove mentions of passwords from the pidgin account dialog

Review Request #572 — Created March 13, 2021 and submitted

Information

pidgin/pidgin
default
32e216120df4

Reviewers

Remove mentions of passwords from the pidgin account dialog

This fixes #PIDGIN-17482

screenshot1 | screenshot2

Description From Last Updated

The menu option you're removing here exists expliclty because XMPP allows in-band password changes. What exactly is the reason to …

rekkanoryorekkanoryo

Please do not comment out unused lines, rather really remove them.

qarkaiqarkai

Sorry I just noticed this now, but it looks like something happened between rev 4 and rev 5 of your …

grimgrim

So the diff looks like it's just the last commit again?

grimgrim

password_box, password_entry and remember_pass_check are uninitialized now. Probably they should be removed from AccountPrefsDialog.

qarkaiqarkai

Looks like trailing whitespace here and on 306.

grimgrim

value still has value from line 1325, i.e. alias.

qarkaiqarkai

looks like there's still some trailing whitespace here.

grimgrim

There is no password field, so there's no reason to set or unset the password here. Please check the entire …

QuLogicQuLogic

This all still has to go away... If there's no password entry in the dialog, the password cannot and should …

QuLogicQuLogic
prateekpardeshi
grim
  1. 
      
  2. This is the buddy list window, not the account dialog..

  3. 
      
rekkanoryo
  1. 
      
  2. Show all issues

    The menu option you're removing here exists expliclty because XMPP allows in-band password changes. What exactly is the reason to remove this? Your comment in the linked issue that "This field is no longer used" is not correct. Not all XMPP servers allow in-band password changes, but that doesn't mean that none do.

    1. So issue is invalid and should be closed as wontfix?

    2. the issue is valid, see my updated comment on the issue. this revirw request modified something completely different.

    3. See this comment for more information. https://issues.imfreedom.org/issue/PIDGIN-17482#focus=Comments-4-83289.0-0

  3. 
      
Robbie
  1. 
      
    1. I will make the required changes, I'm new here please give me a little time :)
      I'll try to fix this ASAP

  2. Should this pull request be closed?

    1. An updated version could contain the proper fix.

  3. 
      
prateekpardeshi
qarkai
  1. 
      
  2. pidgin/gtkaccount.c (Diff revision 2)
     
     
    Show all issues

    password_box, password_entry and remember_pass_check are uninitialized now. Probably they should be removed from AccountPrefsDialog.

    1. Working on it!

    2. The password_entry mentioned here: https://keep.imfreedom.org/pidgin/pidgin/file/tip/pidgin/gtkaccount.c#l309

      What is it used for? Is it still needed?

    3. Those are the same entries that need to be removed.

  3. 
      
prateekpardeshi
qarkai
  1. 
      
  2. Show all issues

    Please do not comment out unused lines, rather really remove them.

  3. 
      
prateekpardeshi
qarkai
  1. 
      
  2. pidgin/gtkaccount.c (Diff revision 4)
     
     
    Show all issues

    value still has value from line 1325, i.e. alias.

    1. Yes, since value = gtk_entry_get_text(GTK_ENTRY(dialog->password_entry)); has been removed, value would be having the value of alias from line 1325.
      But I think purple_account_get_remember_password(line 1236) would be unused too. Is it correct? Shall I remove that too?

  3. 
      
grim
  1. 
      
  2. pidgin/gtkaccount.c (Diff revision 4)
     
     
    Show all issues

    Looks like trailing whitespace here and on 306.

  3. 
      
prateekpardeshi
grim
  1. 
      
  2. pidgin/gtkaccount.c (Diff revision 5)
     
     
    Show all issues

    looks like there's still some trailing whitespace here.

  3. 
      
prateekpardeshi
grim
  1. 
      
  2. Show all issues

    Sorry I just noticed this now, but it looks like something happened between rev 4 and rev 5 of your changes and the deleting of the widgets from the window aren't in the diff anymore.

    1. I'll figure this one out today

    2. I was pulling my own branch before default, I think this might be the reason. I pulled from default branch this time

  3. 
      
prateekpardeshi
prateekpardeshi
QuLogic
  1. 
      
  2. pidgin/gtkaccount.c (Diff revision 8)
     
     
     
     
     
     
     
    Show all issues

    There is no password field, so there's no reason to set or unset the password here. Please check the entire flow of the function.

  3. 
      
prateekpardeshi
prateekpardeshi
grim
  1. 
      
  2. Show all issues

    So the diff looks like it's just the last commit again?

    1. replaced value with NULL parameter in the latest commit

  3. 
      
prateekpardeshi
grim
  1. Ship It!
  2. 
      
QuLogic
  1. 
      
  2. pidgin/gtkaccount.c (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This all still has to go away... If there's no password entry in the dialog, the password cannot and should not be modified at all.

    1. but without the else here to clear the password the only other way to do so is by deleting the account or via the password manager itself. Which is probably fine?

    2. There's no checkbox for saving passwords here any more, so this won't ever clear the password if it was saved. What it will do is set it to NULL in the first part of the if. Why should opening the Account settings do that?

    3. Good point. Would we want to clear it on new account creation though?

    4. I suppose one might delete and re-add an account and hope the password were saved, but I guess it's not necessarily required, so clearing on might work.

      But maybe we should have explicit Clear and Set buttons, if there's no other way to modify a saved password?

    5. I talked to prateek and we're just going to remove the credential manager code from this function.

  3. 
      
prateekpardeshi
QuLogic
  1. Ship It!
  2. 
      
grim
Review request changed
Status:
Completed