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

Fix approval in manual operations #48

Merged
merged 2 commits into from
May 9, 2024
Merged

Fix approval in manual operations #48

merged 2 commits into from
May 9, 2024

Conversation

guillaume-duong-bib
Copy link
Contributor

@guillaume-duong-bib guillaume-duong-bib commented Apr 30, 2024

Fixes mitre/caldera#2887.
Fixes and closes mitre/caldera#2962

Reasoning for the changes:

  • deletion of line 31:
    • Just by thinking about it, this doesn't make much sense to me, since it basically means "if the link is not finished (does not have a finish date), it is not editable". This also goes against the comment that mentions that links in paused or queued can also be editable.
    • Secondly, this contradicts the last check, because we end up in this situation:
      • (currentLink.finish = False) => (isEditable = False) (from line 31)
      • (currentLink.finish = True) => (isEditable = False) (from line 36), assuming that (currentLink.finish.length > 0) <=> (currentLink.finish). I assume this because I could only find any assignment to link.finish with the value of get_current_timestamp(). Since the initial value of finish is None, this would mean finish either has a value of None or a strictly not empty string. So right-to-left is True, with left-to-right being trivial.
      • Assuming both propositions are true, we always have isEditable = False (and thus the Approval is never possible).
  • modification of line 36:
    • By reformulating !(currentLink.finish.length > 0), we are checking that currentLink.finish is an empty string. But as explained before, this situation does not happen. If finish is not set, it has a value of None/Null and thus .length fails.

The only issue I could imagine with this fix is in the event of finish being able to reference an empty string for specific situations, but I did not find any trace of that ever happening.

Here's what a manual operation looks like with this change:
image

NB: as per mitre/caldera#2968, using this fix will in most cases trigger another error, but I chose to leave it separate as it's a different root cause with a different fix.

@elegantmoose
Copy link
Collaborator

on my queue

@elegantmoose
Copy link
Collaborator

Your logic tracks. Im guessing it was a copy/paste error, using the line above and then forgot to remove the "!". Removing the line all together seems cleaner too given the final compound logic evaluation.

@elegantmoose
Copy link
Collaborator

Requires #49 as well to work.

@elegantmoose
Copy link
Collaborator

Thank you much @guillaume-duong-bib ! 👏

@elegantmoose elegantmoose merged commit 10b5711 into mitre:master May 9, 2024
1 check failed
@guillaume-duong-bib guillaume-duong-bib deleted the fix-manual-approval branch May 13, 2024 07:54
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.

No review command button during empty operation Training Module Cannot start ability manually
2 participants