-
Notifications
You must be signed in to change notification settings - Fork 451
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
Exp checklist permission mismatch bug. #2545
Conversation
Your preview environment pr-2545-bttf has been deployed. Preview environment endpoints are available at: |
const envs = experiment ? getAffectedEnvsForExperiment({ experiment }) : []; | ||
|
||
if (!context.permissions.canRunExperiment(experiment, envs)) { | ||
if (!context.permissions.canViewExperimentModal(experiment.project)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should be canUpdateExperiment
instead, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOH. Yes. I've updated the both places to use the canUpdateExperiment
.
|
||
if (!context.permissions.canRunExperiment(experiment, envs)) { | ||
const updatedExperiment = { ...experiment, manualLaunchChecklist: checklist }; | ||
if (!context.permissions.canUpdateExperiment(experiment, updatedExperiment)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2nd argument should just be the fields that are changing, not the entire updated experiment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment, but it's minor so approving
Features and Changes
Our sentry logs indicated a user received an error when attempting to edit an experiment's checklist.
When digging in, it appeared that this error was thrown for an
analyst
user trying to update a manual checklist item on an experiment's checklist. After some digging, I found that on the frontend, when determining whether the user should have edit access, we were checkingcanViewExperimentModal
which checks thecreateAnalyses
permission, but on the backend, we were checkingcanRunExperiment
which checks therunExperiments
permission.Looking at the roles and policies, an
analyst
is described as being able to create, edit, and delete experiments, so my understanding is the backend check we were doing was checking too high of a permission level.This PR updates the backend to check the same permission that we're checking on the frontend so no users will get UI that says they can update, but the backend says otherwise.
Testing