Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Allow the user to logout with a malformed or expired token [#1257] #1305

Merged
merged 6 commits into from
Sep 14, 2022

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Sep 13, 2022

Purpose

Whenever the user logs out from the Admin UI, the local auth token should be deleted no matter what happens on the server (403, 500, etc.). This would prevent you from getting into an unrecoverable state where you cannot clear your token

Changes

  • Bump fideslib version to raise a 403 on other endpoints besides logout if a token is malformed Handling Invalid Tokens [#1257] fideslib#71
  • Fix issues with logout due to bad/expired tokens
    • Unless a token is not supplied from the logout endpoint, or the client delete fails (currently a 500), return a 204 on logout.
    • Create new security callable to use for logout: logout_oauth_client instead of using the verify_oauth_client from fideslib:
      • If there's no token, throw a 401
      • If token malformed, ignore, and return a 204
      • Skip checking token expiration for logout, this shouldn't effect if we can get rid of this token
      • If there's no client on the token, ignore, and return a 204
      • If the client on the token doesn't exist in the db, ignore and return a 204
      • If the client exists in the db, delete, and return a 204.
    • I don't distingush between error codes on logout - 204 or 200 depending on whether client was deleted or not, just straight 204's across the board, unless no token is supplied.
    • I believe this successful 204 response lets the token get deleted from local storage too

Testing

See testing instructions in #1257

Checklist

  • Update CHANGELOG.md file
    • Merge in main so the most recent CHANGELOG.md file is being appended to
    • Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
    • Add a link to this PR at the end of the description with the PR number as the text. example: #1
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #1257

@pattisdr pattisdr changed the title [DRAFT] Allow the user to logout with a malformed or expired token [#1257] Allow the user to logout with a malformed or expired token [#1257] Sep 13, 2022
@pattisdr pattisdr marked this pull request as ready for review September 13, 2022 20:38
@sanders41
Copy link
Contributor

sanders41 commented Sep 14, 2022

I still got the error when logging out with the root user. I wonder if this one is specific to root user. It is using the root client, could logging out be trying to delete the root client and causing and error because of that? Either that or with the root user there is no user in the database, could that be throwing things off?

Screenshot from 2022-09-13 20-49-22

Logs from the server:

fidesops-webserver-1  | ERROR:uvicorn.error:Exception in ASGI application
fidesops-webserver-1  | Traceback (most recent call last):
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/uvicorn/protocols/http/httptools_impl.py", line 372, in run_asgi
fidesops-webserver-1  |     result = await app(self.scope, self.receive, self.send)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/uvicorn/middleware/proxy_headers.py", line 75, in __call__
fidesops-webserver-1  |     return await self.app(scope, receive, send)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/uvicorn/middleware/message_logger.py", line 82, in __call__
fidesops-webserver-1  |     raise exc from None
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/uvicorn/middleware/message_logger.py", line 78, in __call__
fidesops-webserver-1  |     await self.app(scope, inner_receive, inner_send)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/fastapi/applications.py", line 269, in __call__
fidesops-webserver-1  |     await super().__call__(scope, receive, send)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/applications.py", line 124, in __call__
fidesops-webserver-1  |     await self.middleware_stack(scope, receive, send)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/middleware/errors.py", line 184, in __call__
fidesops-webserver-1  |     raise exc
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/middleware/errors.py", line 162, in __call__
fidesops-webserver-1  |     await self.app(scope, receive, _send)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/middleware/base.py", line 68, in __call__
fidesops-webserver-1  |     response = await self.dispatch_func(request, call_next)
fidesops-webserver-1  |   File "/fidesops/src/fidesops/main.py", line 80, in dispatch_log_request
fidesops-webserver-1  |     response = await call_next(request)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/middleware/base.py", line 46, in call_next
fidesops-webserver-1  |     raise app_exc
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/middleware/base.py", line 36, in coro
fidesops-webserver-1  |     await self.app(scope, request.receive, send_stream.send)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/middleware/cors.py", line 92, in __call__
fidesops-webserver-1  |     await self.simple_response(scope, receive, send, request_headers=headers)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/middleware/cors.py", line 147, in simple_response
fidesops-webserver-1  |     await self.app(scope, receive, send)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/exceptions.py", line 93, in __call__
fidesops-webserver-1  |     raise exc
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/exceptions.py", line 82, in __call__
fidesops-webserver-1  |     await self.app(scope, receive, sender)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/fastapi/middleware/asyncexitstack.py", line 21, in __call__
fidesops-webserver-1  |     raise e
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/fastapi/middleware/asyncexitstack.py", line 18, in __call__
fidesops-webserver-1  |     await self.app(scope, receive, send)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/routing.py", line 670, in __call__
fidesops-webserver-1  |     await route.handle(scope, receive, send)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/routing.py", line 266, in handle
fidesops-webserver-1  |     await self.app(scope, receive, send)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/routing.py", line 65, in app
fidesops-webserver-1  |     response = await func(request)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/fastapi/routing.py", line 231, in app
fidesops-webserver-1  |     raw_response = await run_endpoint_function(
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/fastapi/routing.py", line 162, in run_endpoint_function
fidesops-webserver-1  |     return await run_in_threadpool(dependant.call, **values)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/starlette/concurrency.py", line 41, in run_in_threadpool
fidesops-webserver-1  |     return await anyio.to_thread.run_sync(func, *args)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/anyio/to_thread.py", line 31, in run_sync
fidesops-webserver-1  |     return await get_asynclib().run_sync_in_worker_thread(
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/anyio/_backends/_asyncio.py", line 937, in run_sync_in_worker_thread
fidesops-webserver-1  |     return await future
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/anyio/_backends/_asyncio.py", line 867, in run
fidesops-webserver-1  |     result = context.run(func, *args)
fidesops-webserver-1  |   File "/fidesops/src/fidesops/ops/api/v1/endpoints/user_endpoints.py", line 160, in user_logout
fidesops-webserver-1  |     client.delete(db)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/fideslib/db/base_class.py", line 259, in delete
fidesops-webserver-1  |     db.delete(self)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/sqlalchemy/orm/session.py", line 2575, in delete
fidesops-webserver-1  |     self._delete_impl(state, instance, head=True)
fidesops-webserver-1  |   File "/usr/local/lib/python3.9/site-packages/sqlalchemy/orm/session.py", line 2581, in _delete_impl
fidesops-webserver-1  |     raise sa_exc.InvalidRequestError(
fidesops-webserver-1  | sqlalchemy.exc.InvalidRequestError: Instance '<ClientDetail at 0x7f057050f2e0>' is not persisted

@pattisdr
Copy link
Contributor Author

Great callout @sanders41 - I hadn't tested against the root user.

Copy link
Contributor

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

Great job on getting this fixed, it had been causing lots of people issues.

@sanders41 sanders41 merged commit 2598adf into main Sep 14, 2022
@sanders41 sanders41 deleted the fidesops_1257_logout_issues branch September 14, 2022 11:36
sanders41 pushed a commit that referenced this pull request Sep 22, 2022
…1305)

* Allow the user to logout with a malformed or expired token.

* Fix formatting.

* Fix test comment.

* Update changelog.

* Bump fideslib version to raise a 403 if the supplied token is malformed instead of a 500.

* Allow the root user to logout.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to logout from Admin UI with an expired/invalid token
2 participants