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

Restrict updates to metadata access field #1954

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

jjnesbitt
Copy link
Member

@jjnesbitt jjnesbitt commented Jun 6, 2024

Closes #1787
Closes #1831

It seems both these issues were due to us not prohibiting the access field from being updated. A user should never update the access field (as it is derived from the embargo status of the dandiset), nor is there a natural way for that to happen using the meditor. However, it was not actually disallowed in anyway, so the following situations were possible:

  1. After the dandiset is embargoed, if the meditor uses old metadata in some way, it can overwrite this field with the old embargoed access field. All without the user being aware, since that field isn't available in the meditor
  2. If someone were to simply use the API outside of the meditor, they could set that field to whatever they want, intentional or not.

This PR updates that field to treat it as computed and thus not able to be changed from a simple metadata update. (See below)

waxlamp
waxlamp previously approved these changes Jun 13, 2024
Copy link
Member

@waxlamp waxlamp left a comment

Choose a reason for hiding this comment

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

I had two questions, but if the answers are what I think, they don't block approval and merge of this PR:

  1. The new test that checks for non-mutability of the access field relies solely on the addition of access to the computed_fields list in Version.strip_metadata(), right?
  2. There is no added test to see whether the correct access field value pops out of an embargoed Dandiset--is that already covered by existing tests (i.e., those tests unaffected by adding access as an immutable field)?

As I said, aside from those questions... LGTM.

@jjnesbitt jjnesbitt force-pushed the dont-allow-set-embargo-status branch from cfcd508 to c832cd7 Compare June 13, 2024 22:51
@jjnesbitt jjnesbitt requested a review from waxlamp June 13, 2024 22:52
@jjnesbitt
Copy link
Member Author

jjnesbitt commented Jun 13, 2024

This PR has changed slightly, as I realized that there are sometimes other sub-fields in access, that may indeed require updating (contactPoint is one such example). As such, I've updated this change to instead restrict updates to access to be in the correct format, while still allowing extra fields to be present. So access is not really being treated as "computed", rather is just handled in a special way.

@jjnesbitt jjnesbitt changed the title Don't allow updating metadata access field Restrict updates to metadata access field Jun 13, 2024
@jjnesbitt jjnesbitt dismissed waxlamp’s stale review June 14, 2024 15:12

Changed implementation

@jjnesbitt jjnesbitt added patch Increment the patch version when merged release Create a release when this pr is merged labels Jun 14, 2024
Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

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

Just one improvement that I think can be made to a comment, otherwise this makes sense and lgtm

dandiapi/api/models/version.py Outdated Show resolved Hide resolved
@jjnesbitt jjnesbitt force-pushed the dont-allow-set-embargo-status branch from aa4916e to 070d42d Compare June 18, 2024 20:40
@jjnesbitt jjnesbitt merged commit 658836f into master Jun 18, 2024
11 checks passed
@jjnesbitt jjnesbitt deleted the dont-allow-set-embargo-status branch June 18, 2024 21:36
@dandibot
Copy link
Member

🚀 PR was released in v0.3.86 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged released This issue/pull request has been released.
Projects
None yet
4 participants