Skip to content

fix: Add access-control for admin-specific route #5942

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 23, 2019
Merged

fix: Add access-control for admin-specific route #5942

merged 1 commit into from
May 23, 2019

Conversation

@mrsaicharan1
Copy link
Member

@mrsaicharan1 mrsaicharan1 commented May 21, 2019 •
edited
Loading

env-access

Fixes #5923

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

Short description of what this resolves:

Fixes the vulnerability issue alluding to the credentials and environment details which are exposed to non-admin users.

Changes proposed in this pull request:

  • Create a route to serve details of environment
  • Add is_admin decorator to check for access

houndci-bot
houndci-bot reviewed May 21, 2019
View reviewed changes
@mrsaicharan1 mrsaicharan1 force-pushed the cred-leak-fix branch 2 times, most recently from b446a77 to e2020af Compare May 21, 2019 12:29
@mrsaicharan1
Copy link
Member Author

mrsaicharan1 commented May 21, 2019

@iamareebjamal Please check

@codecov
Copy link

codecov bot commented May 21, 2019 •
edited
Loading

Codecov Report

Merging #5942 into development will decrease coverage by 0.04%.
The diff coverage is 48%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development   #5942      +/-   ##
==============================================
- Coverage        66.44%   66.4%   -0.05%     
==============================================
  Files              285     285              
  Lines            13871   13893      +22     
==============================================
+ Hits              9216    9225       +9     
- Misses            4655    4668      +13
Impacted Files Coverage Δ
app/__init__.py 86.3% <100%> (ø) ⬆️
app/api/auth.py 21.39% <40.9%> (+2.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8edfddb...89e1b28. Read the comment docs.

@iamareebjamal
Copy link
Member

iamareebjamal commented May 21, 2019

HTTP Basic Auth should work with this. Please handle that

@mrsaicharan1
Copy link
Member Author

mrsaicharan1 commented May 21, 2019 •
edited
Loading

HTTP Basic Auth should work with this. Please handle that

HTTP basic auth as in? Can you please elaborate?

@iamareebjamal
Copy link
Member

iamareebjamal commented May 21, 2019

https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication

http://flask.pocoo.org/snippets/8/

@mrsaicharan1
Copy link
Member Author

mrsaicharan1 commented May 21, 2019 •
edited
Loading

@iamareebjamal Should the user be redirected to the authentication page when they're not logged in as an admin?Also, Isn't the basic HTTP auth working now?

@iamareebjamal
Copy link
Member

iamareebjamal commented May 21, 2019

No, there'll be no redirection

https://www.httpwatch.com/httpgallery/authentication/#showExample10

houndci-bot
houndci-bot reviewed May 21, 2019
View reviewed changes
@mrsaicharan1 mrsaicharan1 force-pushed the cred-leak-fix branch 2 times, most recently from fc3f78d to 9e7f96a Compare May 21, 2019 16:29
@iamareebjamal iamareebjamal changed the title Fix: Add access-control for admin-specific route fix: Add access-control for admin-specific route May 21, 2019
@auto-label auto-label bot added the fix label May 21, 2019
@iamareebjamal
Copy link
Member

iamareebjamal commented May 21, 2019

Please verify that it is working or not

@mrsaicharan1 mrsaicharan1 force-pushed the cred-leak-fix branch 2 times, most recently from 1ba0852 to 3b88c50 Compare May 22, 2019 21:04
houndci-bot
houndci-bot reviewed May 22, 2019
View reviewed changes
app/api/auth.py
return f(*args, **kwargs)
return decorated

@authorised_blueprint.route('/environment')
Copy link

@houndci-bot houndci-bot May 22, 2019

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

app/api/auth.py
return ForbiddenError({'source': ''}, 'Authentication Required to access Invoice').respond()

# Access for Environment details & Basic Auth Support
def check_auth_admin(username, password):
Copy link

@houndci-bot houndci-bot May 22, 2019

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

@mrsaicharan1
Copy link
Member Author

mrsaicharan1 commented May 22, 2019 •
edited
Loading

@iamareebjamal I've add proper support now which handles the admin access. I've also attached a GIF to the description. It works fine now.
Previously, the decorators were not working inside of app/__init__.py. I've noticed this in my previous PRs as well. Had to shift the EnvironmentDump object from there to control the access.

@fossasia fossasia deleted a comment May 22, 2019
@fossasia fossasia deleted a comment May 22, 2019
@fossasia fossasia deleted a comment May 23, 2019
@fossasia fossasia deleted a comment May 23, 2019
@iamareebjamal iamareebjamal merged commit e261d23 into fossasia:development May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iamareebjamal iamareebjamal iamareebjamal approved these changes

+1 more reviewer

@houndci-bot houndci-bot houndci-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Credentials Exposed to non-admin users

3 participants

@mrsaicharan1 @iamareebjamal @houndci-bot