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

Return 403 instead of 404 for private published documents #135

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

kdid
Copy link
Contributor

@kdid kdid commented Mar 20, 2023

  • Return 403 instead of 404 for private published documents (this includes Works, FileSets and Collections responses.)

@kdid kdid requested review from mbklein and adamjarling March 20, 2023 15:45
Copy link
Contributor

@adamjarling adamjarling left a comment

Choose a reason for hiding this comment

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

Code looks good. A few small syntaxy things, but looks solid

statusCode: 404,
body: JSON.stringify(responseBody),
};
if (index != "shared_links") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (index != "shared_links") {
if (index !== "shared_links") {

There's not a reason to do a strict check here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh probably not. Good catch.

}),
};
} else if (!isVisible(body, opts)) {
if (body?._source.published) {
Copy link
Contributor

@adamjarling adamjarling Mar 20, 2023

Choose a reason for hiding this comment

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

If ya want, could re-write this block like:

response = body?._source.published 
  ? { statusCode: 403 }
  : {
      statusCode: 404,
      body: JSON.stringify({
         _index: prefix(index),
         _type: "_doc",
         _id: id,
         found: false,
      })
  };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@kdid kdid force-pushed the 3754-403-not-404 branch from cd01a80 to 3dbbbd0 Compare March 20, 2023 17:15
@kdid kdid temporarily deployed to test March 20, 2023 17:15 — with GitHub Actions Inactive
@kdid kdid force-pushed the 3754-403-not-404 branch from 3dbbbd0 to d32d701 Compare March 20, 2023 17:19
@kdid kdid temporarily deployed to test March 20, 2023 17:19 — with GitHub Actions Inactive
@kdid
Copy link
Contributor Author

kdid commented Mar 20, 2023

@adamjarling - updated. Thanks.

Copy link
Contributor

@adamjarling adamjarling left a comment

Choose a reason for hiding this comment

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

Looking good

@kdid kdid merged commit 7bd2752 into deploy/staging Mar 20, 2023
@kdid kdid deleted the 3754-403-not-404 branch March 20, 2023 21:16
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