{"action":"opened","issue":{"url":"https://api.github.com/repos/magfest/ubersystem/issues/1923","repository_url":"https://api.github.com/repos/magfest/ubersystem","labels_url":"https://api.github.com/repos/magfest/ubersystem/issues/1923/labels{/name}","comments_url":"https://api.github.com/repos/magfest/ubersystem/issues/1923/comments","events_url":"https://api.github.com/repos/magfest/ubersystem/issues/1923/events","html_url":"https://github.com/magfest/ubersystem/issues/1923","id":169591529,"number":1923,"title":"Test for 'is this admin user logged in' not bulletproof if the session.account_id is set but no longer valid","user":{"login":"binary1230","id":5413064,"avatar_url":"https://avatars.githubusercontent.com/u/5413064?v=3","gravatar_id":"","url":"https://api.github.com/users/binary1230","html_url":"https://github.com/binary1230","followers_url":"https://api.github.com/users/binary1230/followers","following_url":"https://api.github.com/users/binary1230/following{/other_user}","gists_url":"https://api.github.com/users/binary1230/gists{/gist_id}","starred_url":"https://api.github.com/users/binary1230/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/binary1230/subscriptions","organizations_url":"https://api.github.com/users/binary1230/orgs","repos_url":"https://api.github.com/users/binary1230/repos","events_url":"https://api.github.com/users/binary1230/events{/privacy}","received_events_url":"https://api.github.com/users/binary1230/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2016-08-05T11:57:19Z","updated_at":"2016-08-05T11:57:19Z","closed_at":null,"body":"This is mostly an edge case so not too worried about it, and we do handle this decently gracefully but...\r\n\r\nWe often do tests for whether a user is logged in by doing something like the following:\r\n```\r\nif cherrypy.session.get('account_id') is None:\r\n    raise HTTPRedirect('../accounts/login?message=You+are+not+logged+in')\r\n```\r\n\r\nHowever, it's insufficient to test only that, and can lead to weird (but valid) end-user experiences when their admin account is deleted while they were logged in.  Typically we only see that when doing development and we do not normal things like resetting the database while logged in.  \r\n\r\nTake the following scenario:\r\n1) login as admin user, which means ```cherrypy.session['account_id']```, which comes from a cookie, is set to the ID of an AdminAccount in the database\r\n2) delete that AdminAccount (either by deleting the AdminAccount in the UI while logged in as someone else, by resetting the DB with ```sep reset_uber_db```), or manually modifying the cookie to set account_id to an invalid value.\r\n\r\nThe result is that ```cherrypy.session['account_id']``` is now set to an invalid value.  \r\nEXPECTED: Having an invalid value should signal ubersystem to treat this user as no longer logged in, and log you out\r\nACTUAL: Our checks see that account_id is set to something (even if invalid) and considers the user logged in.  This ends up not being a security issue because later access control checks catch this and the user can't access anything (seeing messages like 'you need Registration access to view this page').  However, it's kind of confusing and jarring.\r\n\r\nThe true solution here, if we're OK with another database call at the top of every page, is to query if session['account_id'] is a valid AdminAccount (by doing perhaps something like ```admin_account(cherrypy.session['account_id']) is not None```, and if it's not valid, to call our logout function.\r\n\r\nUltimately low priority and not hurting anything, just occasionally weird and confusing."}}
{"actor":{"display_login":"binary1230"}}