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

Add more information when returning 403 #833

Merged
merged 3 commits into from
Aug 15, 2019
Merged

Conversation

atemate
Copy link
Contributor

@atemate atemate commented Aug 7, 2019

Motivation: when one executes neuro run/log/top/delete with multiple resources used (image, volumes), we raise just web.HTTPForbidden without any additional information on which exactly resource is forbidden.

In future, we will need to patch all errors we raise server-side to provide more information there (i.e. raise web.HTTPForbidden() does not include any body, so we should get rid of it).

@atemate atemate requested review from astaff and dalazx August 7, 2019 22:26
@atemate atemate requested a review from asvetlov August 7, 2019 22:31
@atemate atemate changed the title Add more information when returning 403 [WIP] Add more information when returning 403 Aug 8, 2019
@codecov
Copy link

codecov bot commented Aug 9, 2019

Codecov Report

Merging #833 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #833      +/-   ##
==========================================
+ Coverage   94.36%   94.38%   +0.01%     
==========================================
  Files          30       30              
  Lines        3921     3933      +12     
  Branches      434      436       +2     
==========================================
+ Hits         3700     3712      +12     
  Misses        166      166              
  Partials       55       55
Impacted Files Coverage Δ
platform_api/handlers/jobs_handler.py 98.03% <100%> (+0.06%) ⬆️
platform_api/user.py 94.59% <100%> (+0.3%) ⬆️

@codecov
Copy link

codecov bot commented Aug 9, 2019

Codecov Report

Merging #833 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #833      +/-   ##
==========================================
+ Coverage   94.36%   94.37%   +0.01%     
==========================================
  Files          30       30              
  Lines        3921     3929       +8     
  Branches      434      436       +2     
==========================================
+ Hits         3700     3708       +8     
  Misses        166      166              
  Partials       55       55
Impacted Files Coverage Δ
platform_api/handlers/jobs_handler.py 98% <100%> (+0.04%) ⬆️
platform_api/user.py 94.59% <100%> (+0.3%) ⬆️

@atemate atemate changed the title [WIP] Add more information when returning 403 Add more information when returning 403 Aug 9, 2019
Copy link
Collaborator

@dalazx dalazx left a comment

Choose a reason for hiding this comment

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

not sure if we want to completely stop using aiohttp_security.check_permission. have we checked our options re extending our auth policies etc?

@atemate
Copy link
Contributor Author

atemate commented Aug 9, 2019

have we checked our options re extending our auth policies etc?

@asvetlov is it reasonable to extend aiohttp_security's functionality so that AuthPolicy so that method async def permits(...) -> bool is more general something like async def permits(...) -> Any OR aiohttp_security provides specific general exceptions that contain additional information?

@atemate
Copy link
Contributor Author

atemate commented Aug 14, 2019

created an issue on aiohttp_security: aio-libs/aiohttp-security#241

@atemate
Copy link
Contributor Author

atemate commented Aug 14, 2019

In any case, if we decide to fix this in aiohttp_security first, it will take long. So regarding this PR there are 2 possible ways:

  1. Keep using aiohttp_security. Change the semantics of permits() to return missing permissions (on platform-auth-client side) and wrap it with a custom check_permissions() method temporarily.
  2. Merge it as it is now (and drop dependency on aiohttp_security), and once the aiohttp_security is improved, and return it back

Personally, I'd stick to the first option.

Copy link
Collaborator

@dalazx dalazx left a comment

Choose a reason for hiding this comment

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

@ayushkovskiy please create an issue for moving these changes to neuro-auth-client.

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

Successfully merging this pull request may close these issues.

3 participants