Summary

Remove mentions of passwords from the pidgin account dialog

Review Request #572 — Created March 13, 2021 and submitted July 2, 2021, 1:27 a.m.

Information

Owner

prateekpardeshi

Repository

pidgin/pidgin

Branch

default

Bugs

PIDGIN-17482

Depends On

Commit

32e216120df4

Reviewers

Groups

pidgin

People

Description

Remove mentions of passwords from the pidgin account dialog

Testing Done

This fixes #PIDGIN-17482


  |

Issues

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 …	rekkanoryo	March 18, 2021, 1:36 p.m.
Please do not comment out unused lines, rather really remove them.	qarkai	March 19, 2021, 10:06 a.m.
Sorry I just noticed this now, but it looks like something happened between rev 4 and rev 5 of your …	grim	April 17, 2021, 2:52 a.m.
So the diff looks like it's just the last commit again?	grim	June 16, 2021, 6:27 a.m.
password_box, password_entry and remember_pass_check are uninitialized now. Probably they should be removed from AccountPrefsDialog.	qarkai	March 19, 2021, 10:01 a.m.
Looks like trailing whitespace here and on 306.	grim	April 4, 2021, 8:09 p.m.
value still has value from line 1325, i.e. alias.	qarkai	April 4, 2021, 8:09 p.m.
looks like there's still some trailing whitespace here.	grim	April 6, 2021, 3:46 a.m.
There is no password field, so there's no reason to set or unset the password here. Please check the entire …	QuLogic	May 11, 2021, 8:14 a.m.
This all still has to go away... If there's no password entry in the dialog, the password cannot and should …	QuLogic	June 26, 2021, 7:20 a.m.

Summary:: Remove mentions of passwords from the pidgin account dialog issue fix #PIDGIN-17482
Remove mentions of passwords from the pidgin account dialog
Description:: ~
Remove mentions of passwords from the pidgin account dialog issue fix #PIDGIN-17482
~
Remove mentions of passwords from the pidgin account dialog
Bugs:: PIDGIN-17482

This is the buddy list window, not the account dialog..

The issue has been resolved. 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.

qarkai March 17, 2021, 6:01 p.m.

So issue is invalid and should be closed as wontfix?

grim March 17, 2021, 9:20 p.m.

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

grim March 17, 2021, 10:10 p.m.

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

prateekpardeshi March 17, 2021, 12:14 p.m.

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

Should this pull request be closed?

grim March 17, 2021, 11:20 a.m.

An updated version could contain the proper fix.

Change Summary:

Removed passworrd mention from the account dialogue

Testing Done:

		This fixes #PIDGIN-17482

~
	~
	+
	+
	+
	+	\|

Commit:

04570c687376

7da80985b4f0

Diff:

Revision 2 (+15 -15)

Show changes

pidgin/gtkaccount.c

pidgin/gtkaccount.c (Diff revision 2)

The issue has been resolved. Show all issues

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

prateekpardeshi March 18, 2021, 3:04 p.m.
```
Working on it!
```

prateekpardeshi March 18, 2021, 7:17 p.m.

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?

grim March 19, 2021, 12:44 a.m.

Those are the same entries that need to be removed.

Change Summary:

Removal of password_box, password_entry and remember_pass_check from gtkaccount

Commit:

7da80985b4f0

193e3e15c324

Diff:

Revision 3 (+53 -53)

Show changes

pidgin/gtkaccount.c

The issue has been resolved. Show all issues
```
Please do not comment out unused lines, rather really remove them.
```
1. prateekpardeshi March 19, 2021, 10:01 a.m.
  Making changes now :)
2. prateekpardeshi March 19, 2021, 10:08 a.m.
  Fixed! :)

Change Summary:

Removed unused & commented code

Commit:

193e3e15c324

df72a3c90540

Diff:

Revision 4 (+2 -55)

Show changes

pidgin/gtkaccount.c

pidgin/gtkaccount.c (Diff revision 4)

The issue has been resolved. Show all issues

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

prateekpardeshi March 19, 2021, 10:37 a.m.

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?

pidgin/gtkaccount.c (Diff revision 4)
The issue has been resolved. Show all issues
```
Looks like trailing whitespace here and on 306.
```

Change Summary:

Removal of white spaces and value from line 1325(alias)

Commit:

df72a3c90540

b23f768bf7d2

Diff:

Revision 5 (+1 -20)

Show changes

pidgin/gtkaccount.c

pidgin/gtkaccount.c (Diff revision 5)
The issue has been resolved. Show all issues
```
looks like there's still some trailing whitespace here.
```

Change Summary:

Removed trailing whitespace on line 305

Commit:

b23f768bf7d2

aa54fe44d03a

Diff:

Revision 6 (-19)

Show changes

pidgin/gtkaccount.c

The issue has been resolved. 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.

prateekpardeshi April 16, 2021, 5:46 a.m.
```
I'll figure this one out today
```

prateekpardeshi April 17, 2021, 2:51 a.m.

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

Change Summary:

Deleted password field from the latest updated pull

Commit:

aa54fe44d03a

14349a16b04c

Diff:

Revision 7 (+1 -58)

Show changes

pidgin/gtkaccount.c

Change Summary:

Deleted password field from the latest updated pull (Minor Change)

Commit:

14349a16b04c

dd0a43feeeca

Diff:

Revision 8 (+1 -58)

Show changes

pidgin/gtkaccount.c

pidgin/gtkaccount.c (Diff revision 8)

The issue has been resolved. 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.

Change Summary:

Removed comment regarding set or unset of password

Commit:

dd0a43feeeca

634a54ee2541

Diff:

Revision 9 (-7)

Show changes

pidgin/gtkaccount.c

Change Summary:

Removed value parameter from the purple_credential_manager_write_password_async due to absence of password field

Commit:

634a54ee2541

51ed26c73735

Diff:

Revision 10 (+1 -1)

Show changes

pidgin/gtkaccount.c

The issue has been dropped. Show all issues

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

prateekpardeshi May 18, 2021, 6:23 p.m.

replaced value with NULL parameter in the latest commit

Change Summary:

After rebase: Remove mentions of passwords from the pidgin account dialog

Commit:

51ed26c73735

32e216120df4

Diff:

Revision 11 (+2 -66)

Show changes

pidgin/gtkaccount.c

Ship it!

```
Ship It!
```

pidgin/gtkaccount.c (Diff revision 11)

The issue has been resolved. 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.

grim June 17, 2021, 9 a.m.

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?

QuLogic June 18, 2021, 1:20 a.m.

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?

grim June 18, 2021, 1:35 a.m.

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

QuLogic June 21, 2021, 8:56 p.m.

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?

grim June 23, 2021, 3:16 a.m.

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

Change Summary:

removed unused variables from gtkaccount.c file

Diff:

Revision 12 (-77)

Show changes

pidgin/gtkaccount.c

Ship it!

```
Ship It!
```

Status:: Completed