Handle staticfiles OSError exceptions

[aminalaee]

Closes #1123 Issue
And can also closes #1124 old PR

This will handle staticfiles exceptions:

• If directory not found throws 404 (file not found was already handled)
• if permission denied on file/folder returns 401
• if any other OSError exceptions happen, returns 500

[aminalaee]

I haven't seen any usage of unittest.mock in the project, but I couldn't find any better way to create not-so-simple OSError exceptions.

[JayH5]

Is there any OSError that might cause this? Maybe a broken symlink or would that just 404?

[aminalaee]

I guess that would be a FileNotFound. I'll give it another try but all other unhandled OSError exceptions happened on writing a file or internal sys calls.

[graingert]

statting a file you can't access gives you PermissionError:

>>> os.stat("/boot/bar")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
PermissionError: [Errno 13] Permission denied: '/boot/bar'

[graingert]

ah you already have a test for that

[aminalaee]

@JayH5 Changed that, please check again if that's what you meant.

[JayH5]

Thanks yes that is what I meant. Sorry for the slow response 😕

I've been thinking about this again and I'm not sure we actually want to return 500 from within the static files implementation. Should we not just allow/raise the exception since it is so unexpected? IMO, the ServerErrorMiddleware (and similar) should really be the only thing in Starlette that returns a 500 response.

[aminalaee]

@JayH5
That makes sense. What about the other exceptions?
Like 404 and 401, can't ExceptionMiddleware handle those?

[JayH5]

You do make a good point 🤔 I'm not sure why we don't raise an HTTPException in those cases. I'd be open to that change (if it's as straightforward as it seems) either in this PR or another.

[aminalaee]

@JayH5 I played around this a bit and changed that.
I think we can raise HTTPException instead of returning PlaintTextResponses but there's a difference. If we raise HTTPException then we need to can't use StaticFiles alone as we need Starlette middlewares to handle the exception which StaticFiles doesn't have.

[JayH5]

Is there any OSError that might cause this? Maybe a broken symlink or would that just 404?

[lovelydinosaur]

@aminalaee - Looks neatly done. Happy to leave this up to your judgement.

[lambdaq]

Heads up for anyone using lookup_path(): it's now changed to a sync method in starlette>=0.17.1

I am using await self.lookup_path in my code this PR breaks it lol.

I know it's my bad to use an undocumented internal method, but I just don't want to handle path on my own.