Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reset the "printdisabled" cookie upon logging out as any user. #215

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ClementineAccount
Copy link

@ClementineAccount ClementineAccount commented Oct 30, 2023

This resets the printdisabled cookies when logging out for any user by setting it to expire and be deleted upon logging out as any user, effectively fixing the mentioned issue "Log in as user" feature does not reset printdisabled cookie #8005

- Resets the printdisabled cookies when logging out for any user.
@ClementineAccount ClementineAccount changed the title Reset 'pd' (internetarchive/openlibrary#8005) Reset the "printdisabled" cookie upon logging out as any user. Oct 30, 2023
@@ -288,6 +288,7 @@ class logout(delegate.page):
path = "/account/logout"

def POST(self):
web.setcookie('pd', "", expires=-1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we login as a different patron using the admin function, are we sure this logs the admin out first? Or does it just login as the new person?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the code where we login to the other patron
https://github.com/internetarchive/openlibrary/blob/master/openlibrary/plugins/admin/code.py#L463-L466

Here is where we need to call either logout (before account.generate_login_code()) or clear the pd cookie

While we're at it sfw is another cookie value we want to nullify.

It's worth noting, both swf and pd are unique settings in Open Library and have nothing to do with the generic infogami framework and so the changes we'll want to make should occur in the Open Library repo, not here -- other projects that use infogami will not want cookies randomly deleted upon logout.

We'll need to update openlibrary such that:

  1. We add a new logout controller to plugins/account.py which unsets these two cookie values (pd, sfw) and then calls the super (i.e. infogami's) logout function downstream.
  2. Update https://github.com/internetarchive/openlibrary/blob/master/openlibrary/plugins/admin/code.py#L463-L466 so that it calls the code for this logout handler.

The code might look like...

    # add to https://github.com/internetarchive/openlibrary/blob/master/openlibrary/accounts/model.py
    def clear_cookies():
        web.setcookie('pd', "", expires=-1)
        web.setcookie('sfw', "", expires=-1)

    # in plugins/upstream/account.py
    class logout(delegate.page):
        path = "/account/logout"
        """Registers a handler that listens to the /account/logout endpoint which would otherwise automatically be caught downstream by infogami. Allows us to add our our logic (such as clearing specific cookies) to logout, prior to being handled by infogami's standard logout procedure
        """
        def POST(self):
            # TODO: We need to confirm whether this is current a GET or POST
            from infogami.core.code import logout as infogami_logout
            clear_cookies()
            infogami_logout().POST()

    # in plugins/admin/code.py
    from openlibrary.account.model import clear_cookies
    def POST_su(self, account):
        clear_cookies()  # import from above
        code = account.generate_login_code()
        web.setcookie(config.login_cookie_name, code, expires="")
        return web.seeother("/")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a new logout controller that calls the super is a sensible option. I can make it as a 'new issue' too once I make the pull request to that as it is slightly beyond the scope of just the pd=1 issue, and then reference pd=1 issue there.

I will start work on it. This PR can be closed as the issue will be resolved directly instead.

@cclauss
Copy link

cclauss commented Nov 1, 2023

Failing python_tests were fixed in #216.

@mekarpeles
Copy link
Member

Note also, the original issue was that when an admin uses the button to login as another patron for debugging purposes, the admins cookies were not being cleared after login. So the original issue is not fixed by modifying infogami logout.

The minimal solution would clear pd in the su_login route 🙂

@ClementineAccount
Copy link
Author

ClementineAccount commented Nov 2, 2023

Edit: The fix in my fork seems to work (clears the pd cookie). I will record a video later of not being able to reproduce the bug with the fixes and attach it into a new pull request.

    # in plugins/admin/code.py
    from openlibrary.account.model import clear_cookies
    def POST_su(self, account):
        clear_cookies()  # import from above
        code = account.generate_login_code()
        web.setcookie(config.login_cookie_name, code, expires="")
        return web.seeother("/")

I find this section a bit unclear @mekarpeles. I was able to find an existing POST_su around line 463.
This, however, was what I thought I could do in my own fork: ClementineAccount/openlibrary@8501f68

I will still need to test this before making a new pull request.

   def POST_su(self, account):
       code = account.generate_login_code()
       #Clear all existing admin cookies before logging in as another user
       clear_cookies()
       web.setcookie(config.login_cookie_name, code, expires="")
       return web.seeother("/")

@mekarpeles
Copy link
Member

The function clear_cookies to my knowledge doesn't exist yet, I provided a snippet of what such a function may do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants