From: "Cole Robinson" <crobinso(a)redhat.com&gt; To: abn(a)redhat.com, python-bugzilla(a)lists.fedorahosted.org Cc: pingou(a)pingoured.fr Sent: Sunday, November 30, 2014 5:46:19 AM Subject: Re: [python-bugzilla] [PATCH] Add is_logged_in utility method. On 11/24/2014 06:42 PM, abn(a)redhat.com wrote: > From: Arun Babu Neelicattu <abn(a)redhat.com&gt; > > --- >   bugzilla/base.py | 20 ++++++++++++++++++++ >   1 file changed, 20 insertions(+) > > diff --git a/bugzilla/base.py b/bugzilla/base.py > index e06223f..7aba942 100644 > --- a/bugzilla/base.py > +++ b/bugzilla/base.py > @@ -697,6 +697,26 @@ class BugzillaBase(object): >           self.password = '' >           self.logged_in = False > > +    def is_logged_in(self): > +        """ > +        Utility method to check if this instance has already been logged > in. > + > +        If the instance has its logged_in attribute set to True, this > method > +        returns true. Otherwise, to test if this session is authenticated, > the > +        method calls the User.get() XMLRPC method with ids set. Logged-out > users > +        cannot pass the 'ids' parameter and will result in a 505 error. > +        """ > +        try: > +            if self.logged_in: > +                return True > +            self._proxy.User.get({'ids': self._listify([])}) listify is redundant here since we know we are passing a list. (listify is just a helper to turn None into a list, where it's required)

> +            return True > +        except Fault: > +            e = sys.exc_info()[1] > +            if e.faultCode == 505: > +                return False > +            raise e > + > >       ############################################# >       # Fetching info about the bugzilla instance # > Having this capability is definitely useful. But a few ideas: - We should file a bug with upstream bugzilla to actually provide a real API for this. Bugzilla 5.0 may actually have an API for this... I recall some change WRT login state, but I'm not certain. I can check next week

- I think there's places in the test suite where we already try to approximate this, grep _check_rh_privs. Might be able to use this as well. It might not fit though, but regardless I'd like to have some sort of unit test for this

- After this we will have an API disparity: there's self.logged_in which still has confusing semantics as pingou pointed out. So maybe turn self.logged_in into a readonly property that will call this function, and cache the value. Dunno if it's worth it

- Cole

From: "Cole Robinson" <crobinso(a)redhat.com&gt; To: "Arun Babu Neelicattu" <abn(a)redhat.com&gt; Cc: python-bugzilla(a)lists.fedorahosted.org, pingou(a)pingoured.fr Sent: Thursday, December 11, 2014 11:22:19 AM Subject: Re: [python-bugzilla] [PATCH] Add is_logged_in utility method. On 11/29/2014 10:46 PM, Arun Babu Neelicattu wrote: > > > ----- Original Message ----- >> From: "Cole Robinson" <crobinso(a)redhat.com&gt; >> To: abn(a)redhat.com, python-bugzilla(a)lists.fedorahosted.org >> Cc: pingou(a)pingoured.fr >> Sent: Sunday, November 30, 2014 5:46:19 AM >> Subject: Re: [python-bugzilla] [PATCH] Add is_logged_in utility method. >> >> On 11/24/2014 06:42 PM, abn(a)redhat.com wrote: >>> From: Arun Babu Neelicattu <abn(a)redhat.com&gt; >>> >>> --- >>> bugzilla/base.py | 20 ++++++++++++++++++++ >>> 1 file changed, 20 insertions(+) >>> >>> diff --git a/bugzilla/base.py b/bugzilla/base.py >>> index e06223f..7aba942 100644 >>> --- a/bugzilla/base.py >>> +++ b/bugzilla/base.py >>> @@ -697,6 +697,26 @@ class BugzillaBase(object): >>> self.password = '' >>> self.logged_in = False >>> >>> + def is_logged_in(self): >>> + """ >>> + Utility method to check if this instance has already been logged >>> in. >>> + >>> + If the instance has its logged_in attribute set to True, this >>> method >>> + returns true. Otherwise, to test if this session is >>> authenticated, >>> the >>> + method calls the User.get() XMLRPC method with ids set. >>> Logged-out >>> users >>> + cannot pass the 'ids' parameter and will result in a 505 error. >>> + """ >>> + try: >>> + if self.logged_in: >>> + return True >>> + self._proxy.User.get({'ids': self._listify([])}) >> >> listify is redundant here since we know we are passing a list. (listify is >> just a helper to turn None into a list, where it's required) > > doh! ack > >> >>> + return True >>> + except Fault: >>> + e = sys.exc_info()[1] >>> + if e.faultCode == 505: >>> + return False >>> + raise e >>> + >>> >>> ############################################# >>> # Fetching info about the bugzilla instance # >>> >> >> Having this capability is definitely useful. But a few ideas: >> >> - We should file a bug with upstream bugzilla to actually provide a real >> API >> for this. Bugzilla 5.0 may actually have an API for this... I recall some >> change WRT login state, but I'm not certain. I can check next week >> > > Agreed, could not find any reference to the 5.0 change though, will wait > before filing anything. > Sorry for the delay. There's a new API User.valid_login queued for bugzilla 5: http://bugzilla.readthedocs.org/en/latest/api/core/v1/user.html#valid-login You pass it a login name and a token, it will tell you if the token is still valid for that user. We could use that to accomplish is_logged_in, but we'd have to extend our token cache to track the login account as well. Maybe just add a comment pointing at the above documentation and we can implement it later if we want. >> - I think there's places in the test suite where we already try to >> approximate >> this, grep _check_rh_privs. Might be able to use this as well. It might >> not >> fit though, but regardless I'd like to have some sort of unit test for >> this >> > > I have added a proper unit test for this test10LoginState. If we merge > these changed we can probably get rid of _check_rh_privs I suppose. > >> - After this we will have an API disparity: there's self.logged_in which >> still >> has confusing semantics as pingou pointed out. So maybe turn >> self.logged_in >> into a readonly property that will call this function, and cache the >> value. >> Dunno if it's worth it >> > > This makes sense, I have dropped the caching since i reckon it is rather > pointless (probably even less correct?). I am assuming users will only > check probably only one at the beginning of the call and not before every > bz.* call. If the latter is the case we can easily add caching by setting > self._logged_in. For now I have made self.logged_in make the Users.get > call every time. > > See proposed changes and tests at > https://github.com/abn/python-bugzilla/commit/bff0df85690d787b7560e5a88a0... Patch looks good to me. If you stick in the bit I mention above and send it here I'll apply it - Cole