Add authorization to Dashboard Controller and include owner #558
Conversation
|
This PR is waiting on #535. Specs needs to be uncommented on DashboardController. |
| # Fully qualify template path to allow concern to be included in different modules |
There was a problem hiding this comment.
@ouranos We had to full qualify the template in order to reuse the DashboardController Concern in the TenantDashboard (See maestrano/opal-webstore#51) Do you see problems with that ?
There was a problem hiding this comment.
The linked PR uses the non-admin concern in an admin controller. Not the best approach as the authorization logics are supposed to be different (not the case in this controller for now - see below)
So don't change the framework but the opal-webstore project
|
Once #535 is merged:
then assign to me for review. |
x4d3
force-pushed
the
feature/tenant-dashboards
branch
from
b693bcd to
570ee8c
Compare
x4d3
changed the title
x4d3
force-pushed
the
feature/tenant-dashboards
branch
from
570ee8c to
fff0362
Compare
x4d3
force-pushed
the
feature/tenant-dashboards
branch
from
fff0362 to
5610e79
Compare
|
@ouranos Ready to be merged |
| @@ -79,21 +90,19 @@ def copy | |||
| private | |||
|
|
|||
| def dashboard(*included) | |||
| # TODO: [APIv2] Improve filtering by owner (owner_type?) | |||
| @dashboard ||= MnoEnterprise::Dashboard.where(owner_id: current_user.id).includes(included).find(params[:id].to_i).first | |||
| @dashboard ||= MnoEnterprise::Dashboard.includes(included).find(params[:id]).first | |||
There was a problem hiding this comment.
Security issue 😱
If you're removing the scoping, you should then look at implementing the TODO: authorization above
There was a problem hiding this comment.
No need to, as it's checked later in the authorize.
There was a problem hiding this comment.
This one?
# TODO: enable authorization
# authorize! :manage_dashboard, dashboard| end | ||
|
|
||
| def dashboards | ||
| # TODO: [APIv2] Improve filtering by owner (owner_type?) | ||
| @dashboards ||= MnoEnterprise::Dashboard.includes(*DASHBOARD_DEPENDENCIES).find(owner_id: current_user.id) | ||
| @dashboards ||= MnoEnterprise::Dashboard.includes(*DASHBOARD_DEPENDENCIES).where(owner_id: current_user.id) |
There was a problem hiding this comment.
Why has the TODO comment been removed?
| # Fully qualify template path to allow concern to be included in different modules |
There was a problem hiding this comment.
The linked PR uses the non-admin concern in an admin controller. Not the best approach as the authorization logics are supposed to be different (not the case in this controller for now - see below)
So don't change the framework but the opal-webstore project
x4d3
force-pushed
the
feature/tenant-dashboards
branch
from
5610e79 to
9b066b1
Compare
x4d3
changed the title
|
@ouranos I've reworked entirely this PR to contains only the change related to the introduction of the owner relationship to Dashboard and reintroducing authorization. |
x4d3
force-pushed
the
feature/tenant-dashboards
branch
from
9b066b1 to
5e3e05f
Compare
There was a problem hiding this comment.
Thanks, much better!
I wonder if we still need to authorize the create as a user could craft malicious request to build some random dashboard.
Should be ok as Impac!/IDM is checking access to the organization data before sending it.
I'd be inclined to wait for #560 to be merged so that can be spec'd
|
@cesar-tonnoir you might want to have a look at this |
Include owner in Dashboard management.
Improve Dashboard Controller to use ability. This unfortunately cannot be spec for now (#560 needs to be merged first)