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

[EASI-4486] system intake doc uploads #2691

Merged
merged 18 commits into from
Jul 17, 2024

Conversation

samoddball
Copy link
Contributor

EASI-4486

Description

  • guard document actions (create, view, delete) to allow only those authorized to make changes

How to test this change

  • sign in with a test EUA with only the EASI_P_USER role
  • create a system intake and attach a document (see note)
  • confirm the document creates (this confirms a REQUESTER can create a doc)
  • view the document (this confirms a REQUESTER can view a doc)
  • log out
  • log in with a different EUA and select EASI_D_GOVTEAM role
  • navigate to the original system intake
  • confirm you can view the document (this confirms an ADMIN can view a doc)
  • attempt to delete the doc - it should fail (this confirms an ADMIN cannot delete a REQUESTER doc)
  • log out
  • log back in with original user with EASI_P_USER role
  • go try to delete the doc - it should succeed (this confirms a REQUESTER can delete their own doc)

Note

i forced the virus scan return by doing this

// GetStatusForSystemIntakeDocument queries S3 for the virus-scanning status of a document with the given s3Key
func GetStatusForSystemIntakeDocument(s3Client *upload.S3Client, s3Key string) (models.SystemIntakeDocumentStatus, error) {
	avStatus, err := s3Client.TagValueForKey(s3Key, upload.AVStatusTagName)
	if err != nil {
		return "", err
	}

	return models.SystemIntakeDocumentStatusAvailable, nil

	// possible tag values come from virus scanning lambda
	if avStatus == "CLEAN" {
		return models.SystemIntakeDocumentStatusAvailable, nil
	} else if avStatus == "INFECTED" {
		return models.SystemIntakeDocumentStatusUnavailable, nil
	} else {
		return models.SystemIntakeDocumentStatusPending, nil
	}
}

PR Author Checklist

  • I have provided a detailed description of the changes in this PR.
  • I have provided clear instructions on how to test the changes in this PR.
  • I have updated tests or written new tests as appropriate in this PR.

PR Reviewer Guidelines

  • It's best to pull the branch locally and test it, rather than just looking at the code online!
  • When approving a PR, provide a reason why you're approving it
    • e.g. "Approving because I tested it locally and all functionality works as expected"
    • e.g. "Approving because the change is simple and matches the Figma design"
  • Don't be afraid to leave comments or ask questions, especially if you don't understand why something was done! (This is often a great time to suggest code comments or documentation updates)
  • Check that all code is adequately covered by tests - if it isn't feel free to suggest the addition of tests.

…nch with only system intake(no verify) fresh branch with only system intake(no verify) fresh branch with only system intake(no verify) fresh branch with only system intake(no verify) fresh branch with only system intake(no verify) fresh branch with only system intake(no verify) fresh branch with only system intake(no verify) fresh branch with only system intake
@samoddball samoddball requested a review from a team as a code owner July 11, 2024 22:28
@samoddball samoddball requested review from ClayBenson94 and removed request for a team July 11, 2024 22:28
@adamodd
Copy link
Contributor

adamodd commented Jul 12, 2024

Not sure if you were able to see the gql error return, but this is the error I get when I try to reproduce:

{
  "errors": [
    {
      "message": "sql: no rows in result set",
      "path": [
        "deleteSystemIntakeDocument",
        "document",
        "url"
      ]
    }
  ],
  "data": {
    "deleteSystemIntakeDocument": {
      "document": null,
      "__typename": "DeleteSystemIntakeDocumentPayload"
    }
  }
}

@adamodd
Copy link
Contributor

adamodd commented Jul 12, 2024

There is also a script command to scan for viruses if you'd like:
scripts/tag_minio_file /easi-app-file-uploads/{uuid}.pdf CLEAN

Some frontend markup has the uuid in a testid attr, but not this one. Had to inspect for table document ids to get the uuid in this case.

Comment on lines +126 to +129
if errors.Is(err, sql.ErrNoRows) {
// if the document was deleted, don't error and break
return nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamodd if you were curious what that issue was. it wasn't on the delete at all. finally realized this morning that it was err no rows on the URL resolver for a system intake document, and that runs after deletion. in main, we go straight to S3 for that URL, but in this auth patch, we first check the DB to see who created the document, but it is gone after deletion!

@samoddball samoddball changed the title Easi 4486/system intake doc uploads [EASI-4486] system intake doc uploads Jul 15, 2024
mynar7
mynar7 previously approved these changes Jul 16, 2024
Copy link
Contributor

@mynar7 mynar7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code works as-is, but I do wonder if we can clean up the signature of canCreate to be in-line with canDelete and canView.

@samoddball samoddball merged commit 877a6dd into main Jul 17, 2024
12 checks passed
@samoddball samoddball deleted the EASI-4486/system_intake_doc_uploads branch July 17, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants