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

SecuredRequestHandler behaviour on error #301

Open
mpapillon opened this issue May 8, 2020 · 3 comments
Open

SecuredRequestHandler behaviour on error #301

mpapillon opened this issue May 8, 2020 · 3 comments

Comments

@mpapillon
Copy link

Methods of tsec.authentication.SecuredRequestHandler intercepts all errors and returns Unauthorized status.
I think it should returns an InternalServerError, because it hides real problems to the clients when something wrong happens.

@zakpatterson
Copy link
Contributor

If some requests generate 404s and some generate 401, and some generate 500, then an unauthorized user would be able to crawl URLs to figure out what endpoints are available or not, or what kind of requests generate these different errors. It's probably better to hide information completely to unauthorized users. In development you have the benefit of implementing some logging to determine if there are errors or not.

Try out the gitter channel too for discussions/questions: https://gitter.im/tsecc/Lobby

@mpapillon
Copy link
Author

I agree that it is better to hide information completely to unauthorized users.

But, with the current behaviour, if the user is authorized or not and when the IO fail, the response from SecuredRequestHandler will always be 401. And the other middlewares can no longer intercept IO errors.

    ME.handleErrorWith(middleware(service)) { e: Throwable =>
      SecuredRequestHandler.logger.error(e)("Caught unhandled exception in authenticated service")
      Kleisli.liftF(OptionT.pure(cachedUnauthorized))
    }

I understand that this is the expected behaviour, I think I will develop my own SecuredRequestHandler for my case.

@zakpatterson
Copy link
Contributor

Being able to configure it in that way makes sense. If you can make it a PR I think it could be valuable.

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

No branches or pull requests

2 participants