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

Cleanup Task to Delete incomplete Interviews #831

Closed
michaelhofrichter opened this issue Dec 11, 2023 · 3 comments · Fixed by #921
Closed

Cleanup Task to Delete incomplete Interviews #831

michaelhofrichter opened this issue Dec 11, 2023 · 3 comments · Fixed by #921
Assignees
Labels
bug Something isn't working

Comments

@michaelhofrichter
Copy link
Collaborator

A cleanup task should be to delete any incomplete interviews for the testing user. There are a couple of different API endpoints that might be useful here:

@plocket plocket added the bug Something isn't working label Dec 14, 2023
plocket added a commit that referenced this issue Dec 28, 2023
* Clarify docassemblecli requirement specifier in actions
* Allow complete flexibility in docassemble cli installer
* Ensure projects get deleted on failed takedowns, harden actions by using `env` to avoid directly evaluating inputs.
* Add to CHANGELOG
* Implement PR suggestions for more precautions
@plocket
Copy link
Collaborator

plocket commented Dec 28, 2023

Closed wrong issue, sorry

@plocket plocket reopened this Dec 28, 2023
@plocket
Copy link
Collaborator

plocket commented Dec 28, 2023

Brainstorming:

This may have an MVP and future improvements. For example, one complication is that one interview could open another interview.

  • MVP: Delete only the interview initially created by the bot for the Scenario
  • or MVP: Delete all interviews created through the "I start the interview" Step. I recommend abstracting this further since I already see the potential for future Steps that go to interviews in different ways.
  • V2: Delete all interviews the bot creates during the whole run of all tests

Is it possible to use the API to give every session a tag so that we can just use the tag to delete all of the user's interviews at once? Maybe using the Project name as the tag? We probably need API access to session_tags(), which I think we'd have to write ourselves. Running as an admin can get any interview, not just the current user's interview, but we can't count on that permission level.

Note: We need to use the auth of the user running the interview, not the auth of the author's API key - sometimes authors "log in" as a different user to test different functionality. I think being in an interview and making a request without a different API key would work (never needed to try it), but can we count on being present in an interview at the time we need to delete it? The user could have navigated away from the interview.

We could change how we navigate to an interview so that each "I start the interview" (or similar) Step opens a new window/tab. When we close up a Scenario, we would delete the interview associated with that window/tab before closing it.

@plocket plocket self-assigned this Jun 8, 2024
@plocket
Copy link
Collaborator

plocket commented Jul 1, 2024

Given:

  • To delete an interview with the API, the user needs to give us an API key to delete with.
  • API keys should have the least permissions possible.
  • We should limit ALKiln as much as is reasonable.

Then:

  • ALKiln should assume the API key has the most limited permissions possible.
  • ALKiln should limit its scope as well. ALKiln should only ever try to have a "user" delete their own interview, not allow an author/account to delete any interview on the server. Use only the api/user/interviews endpoint.

Note: We are deleting the interview with API keys. Deleting robotically is slow (introducing more incidental flake) and will run into complications because at some point folks are going to log in as different users in the same test, or use some other complex behavior, and that will be much harder to account for. This is a nice-to-have, not a critical feature. We don't want to make it burdensome if we can reasonably avoid it.

Additional work our authors will have to do:

Example of an author creating (and getting back) a new API key for a user:

curl -X POST \
  https://apps-dev.suffolklitlab.org/api/user/123/api \
  -H 'Content-Type: application/json' \
  -d '{
    "key": "yourAdminAPIKeyWhateverItIs",
    "method": "none"
  }'

This can be done for absolutely any user account.

Further things to consider:

Warnings to add to the report:

  • optional API key github/config secret name was given, but doesn't exist
  • optional API key env var exists and has a value, but its permission level doesn't have access on the server allow the action (deleting a user's own interviews)
  • optional API key env var exists, but has no value (is an empty string)

New tests to write:

API tests, no warnings

  • Not logged in
  • No API key present fails silently
  • (All valid) From local/github tests, type string, valid
  • Try to delete interview address after failing to go to the interview
  • Test from local/github, type non-string (undefined) (warning handled elsewhere)
  • Test from local/github, type string, 403 (access denied, warning handled elsewhere)
  • Test from local/github, type empty string (also 403) (warning handled elsewhere)
  • From playground, type string (no attempt to delete). This is a hard one to test because it would have to be done manually or with a cron job or something. We don't have that set up yet.

API with warnings

  • Test from playground, type non-string This is a hard one to test because it would have to be done manually or with a cron job or something. We don't have that set up yet.
  • Test from local/github, type string, other error (not sure how to trigger)

Non-API warnings

  • API key var with missing env var (Test from local/github, type non-string (undefined))
  • API key var is an empty string (is this feature necessary or useful?) (403, but that's incidental)
  • API key has no access on server (403)

plocket added a commit that referenced this issue Jul 11, 2024
Close #831, allow author to delete interviews from interview list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants