Lernid

Merge lp://qastaging/~deejay1/lernid/keyring-integration into lp://qastaging/lernid

  1. keyring-integration
  2. Merge into trunk
Proposed by Łukasz Jernaś
Status: Needs review
Proposed branch: lp://qastaging/~deejay1/lernid/keyring-integration
Merge into: lp://qastaging/lernid
Diff against target: 98 lines (+33/-2)
2 files modified
lernid/ConnectDialog.py (+3/-0)
lernid/EventManager.py (+30/-2)
To merge this branch: bzr merge lp://qastaging/~deejay1/lernid/keyring-integration
Reviewer Review Type Date Requested Status
John S. Gruber Disapprove
Michael Budde Pending
Review via email: mp+20265@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Łukasz Jernaś (deejay1) wrote :

Works for me...

Reply
Revision history for this message
John S. Gruber (jsjgruber) wrote :

I'm sorry it took so long for anyone to get back to you. I recognize that you put significant effort into learning the python gnome keyring interface, and then coding and testing your patch. Michael is no longer maintaining Lernid and the project was without anyone to look after its code for a while. I just started.

I reviewed your code and then tried it. I'm afraid that I have significant reservations about what it is doing (and, I guess, about the validity of the bug for Lernid).

It seems to me that Lernid should, first and foremost, care for students inexperienced with Ubuntu and #ubuntu-classroom. It goes to some pains to negotiate an acceptable nick for them that is like the one they requested. It seems to me that the additional password panel is an enhancement for the experienced user who has figured out how to use other irc clients, and who has negotiated how to set a nickserv password. That sounds like you and me, but not like our inexperienced students.

The bug is about an enhancement to that password enhancement, and it has costs:

1. As I understand it, once a student has entered a password he or she must thereafter enter what should be a high complexity password to unlock the default keyring for Lernid to retrieve the nick password. This *might* happen just once a login session, saving the student who really wants to use his or her registered nick some effort for subsequent launches of Lernid during the same login session. If it is the result of a less experienced student who tried entering a password on a lark, they are stuck entering their gnome password at least once a signon session, and maybe more, for the life of their keyring.

2. It adds a dependency to the package.

3. It adds code and complexity

It simply doesn't seem worth it to me.

The basic problem here is not your code--I think your code is sound. The problem is the nature of the enhancement requested in bug LP: #527370. I think I'll wait a couple of days and then mark the bug "won't fix".

I regret that you have waited over a year for a response, much less for a response like this, and I regret that this is the nature of our first contact. I sincerely I hope that I haven't put you off Lernid with my opinion.

review: Disapprove
Reply

Unmerged revisions

183. By Łukasz Jernaś

Fix exceptions, remove unnecessary dictionary

182. By Łukasz Jernaś

Keep NickServ password in GNOME keyring

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lernid/ConnectDialog.py'
2--- lernid/ConnectDialog.py 2010-02-13 15:43:30 +0000
3+++ lernid/ConnectDialog.py 2010-02-27 09:35:21 +0000
4@@ -99,6 +99,9 @@
5 def get_password(self):
6 return self.builder.get_object('password').get_text()
7
8+ def set_password(self, password):
9+ return self.builder.get_object('password').set_text(password)
10+
11 def is_valid_nick(self, nick_te):
12 alpha = 'abcdefghijklmnopqrstuvwxyz'
13 alpha = alpha + alpha.upper()
14
15=== modified file 'lernid/EventManager.py'
16--- lernid/EventManager.py 2010-02-13 15:43:30 +0000
17+++ lernid/EventManager.py 2010-02-27 09:35:21 +0000
18@@ -23,6 +23,7 @@
19 import ConfigParser
20 import urllib
21 import logging
22+import gnomekeyring
23
24 from lernid import ConnectDialog
25 from lernid.PasswordDialog import PasswordDialog
26@@ -59,6 +60,8 @@
27 self._passworddialog = None
28 self._widgets = {}
29 self._read_config()
30+
31+ self._keyring = gnomekeyring.get_default_keyring_sync()
32
33 @classmethod
34 def get_instance(cls):
35@@ -96,7 +99,29 @@
36 Statusbar.push_message(_('Connecting to event'), duration=10)
37 self._isconnected = True
38 self.emit('event-connect', self._event)
39+
40+ # Checks the GNOME keyring for a password for the specified nick
41+ def get_keyring_password(self, nick):
42+ try:
43+ # Reads only the first entry, because the search shouldn't return more
44+ password = gnomekeyring.find_network_password_sync(nick, None, 'irc.freenode.net',
45+ 'Lernid', 'irc')[0]['password']
46+ return password
47+ except gnomekeyring.NoMatchError:
48+ return ''
49+ except gnomekeyring.DeniedError:
50+ logging.debug('Failed to read from keyring')
51+ return ''
52+ return ''
53
54+ # Sets or updates keyring password for the specified nick
55+ def set_keyring_password(self, nick, password):
56+ keyring_data = {'user': nick, 'server': 'irc.freenode.net', 'protocol': 'irc', 'object': 'Lernid'}
57+ try:
58+ gnomekeyring.item_create_sync(self._keyring, gnomekeyring.ITEM_NETWORK_PASSWORD,'Lernid', keyring_data, password, True)
59+ except gnomekeyring.DeniedError:
60+ logging.debug('Failed to add keyring item')
61+
62 def _identify_nick(self, nick, password):
63 server = Server.get_server('irc.freenode.net', nick)
64 identifier = server.identify_nick(password)
65@@ -108,10 +133,10 @@
66 self._passworddialog = PasswordDialog.new()
67 password = self._passworddialog.run()
68 if password != None:
69+ self.set_keyring_password(nick,password)
70 identifier.retry(password)
71 identifier.connect('invalid-password', retry)
72-
73-
74+
75 def connect_dialog(self, widget=None, parent=None):
76 """Show the connect dialog and connect to the resources."""
77 if not self._connectdialog:
78@@ -119,6 +144,7 @@
79 self._connectdialog.set_transient_for(parent)
80 self._connectdialog.connect('response', self._connect_dialog_reponse)
81 self._connectdialog.set_nick(Preferences.get('nick'))
82+ self._connectdialog.set_password(self.get_keyring_password(Preferences.get('nick')))
83 self._connectdialog.set_transient_for(widget)
84 self._connectdialog.show()
85
86@@ -130,6 +156,7 @@
87 self._connect_event(event, nick)
88 password = self._connectdialog.get_password()
89 if password:
90+ self.set_keyring_password(nick, password)
91 self._identify_nick(nick, password)
92
93 def disconnect(self):
94@@ -159,3 +186,4 @@
95 return self._widgets[name]
96 return None
97
98+

Subscribers

People subscribed via source and target branches