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

feat: Add 403 page for forbidden error #16612

Conversation

duynguyenhoang
Copy link
Contributor

@duynguyenhoang duynguyenhoang commented Sep 6, 2021

SUMMARY

Introduce a page for Forbidden error.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before
image

After
ksnip_20211006-014621

TESTING INSTRUCTIONS

  • Enable DASHBOARD_RBAC
  • Login with a user who doesn't have access to a dashboard.
  • Verify that the new 403 page appear.

ADDITIONAL INFORMATION

  • Has associated issue: Fixes Improve 403 Forbidden page when user doesn't have access to Dashboard #16611
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@duynguyenhoang
Copy link
Contributor Author

Hi @junlincc, could you please help with the error403 image?

@codecov
Copy link

codecov bot commented Sep 6, 2021

Codecov Report

Merging #16612 (6e0e1ec) into master (3fe2e6e) will not change coverage.
The diff coverage is 88.88%.

❗ Current head 6e0e1ec differs from pull request most recent head 62e6452. Consider uploading reports for the commit 62e6452 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #16612   +/-   ##
=======================================
  Coverage   76.73%   76.73%           
=======================================
  Files        1003     1003           
  Lines       53875    53875           
  Branches     7289     7289           
=======================================
  Hits        41343    41343           
  Misses      12292    12292           
  Partials      240      240           
Flag Coverage Δ
javascript 70.99% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/utils/decorators.py 92.85% <50.00%> (ø)
superset/views/base.py 76.43% <100.00%> (ø)
superset/views/core.py 75.63% <100.00%> (ø)

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 3fe2e6e...62e6452. Read the comment docs.

@duynguyenhoang duynguyenhoang changed the title feat: Add 403 page for forbidden error feat: Add 403 page for forbidden error [WIP] Sep 7, 2021
@duynguyenhoang duynguyenhoang force-pushed the feat/add-403-page-dashboard branch 2 times, most recently from 01d3af8 to 62e6452 Compare September 7, 2021 17:37
@duynguyenhoang duynguyenhoang changed the title feat: Add 403 page for forbidden error [WIP] feat: Add 403 page for forbidden error Sep 7, 2021
@junlincc
Copy link
Member

@duynguyenhoang hi! do you need a high resolution image for the 403 errors?

@junlincc junlincc added the need:followup Requires followup label Sep 15, 2021
@duynguyenhoang
Copy link
Contributor Author

Hi @junlincc, I think we don't need high resolution for it.
The image the same with 404 would be fine, https://raw.githubusercontent.com/apache/superset/master/superset-frontend/images/error404.png

@junlincc
Copy link
Member

https://www.figma.com/file/el48F3KQ9u6rtKmdZvdb36/Error-Pages?node-id=94%3A503

sorry for the delay reply, here you go! and thanks again for the contribution!

@duynguyenhoang duynguyenhoang force-pushed the feat/add-403-page-dashboard branch 2 times, most recently from 7df7e07 to 7dab8ac Compare October 5, 2021 18:47
@duynguyenhoang
Copy link
Contributor Author

Thank you @junlincc for providing the image. I update the PR and it is ready to review. Sorry to reply you too late.

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 16, 2022
@stale stale bot removed the inactive Inactive for >= 30 days label Apr 22, 2022
@rusackas
Copy link
Member

Thanks for this PR! Closing and re-opening to kick-start CI, and added some reviewers in hopes that we can get this looked into!

@rusackas rusackas closed this Jan 23, 2023
@rusackas rusackas reopened this Jan 23, 2023
@michael-s-molina michael-s-molina removed their request for review April 10, 2023 21:13
@duynguyenhoang
Copy link
Contributor Author

duynguyenhoang commented Aug 7, 2023

Hello,
I was quite busy last few months and haven't had time to contribute more.
I will probably come back with this PR this week.

@john-bodley
Copy link
Member

@duynguyenhoang please let us know if you revisit this PR and we'll review it.

@rusackas
Copy link
Member

rusackas commented Feb 1, 2024

Hoping we can get this followed-up on one day, but for now I'll move it to Draft state.

@rusackas rusackas marked this pull request as draft February 1, 2024 22:32
@rusackas
Copy link
Member

rusackas commented Jun 6, 2024

@duynguyenhoang I assume you're not coming back to this, so I'll close it, but let us know if you ever want to rekindle this effort!

@rusackas rusackas closed this Jun 6, 2024
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.

Improve 403 Forbidden page when user doesn't have access to Dashboard
4 participants