Skip to content

Create revision history entries for secret-changes via API#266

Open
JensH0rnung wants to merge 4 commits into
seibert-media:mainfrom
JensH0rnung:feat/api-audit-metadata
Open

Create revision history entries for secret-changes via API#266
JensH0rnung wants to merge 4 commits into
seibert-media:mainfrom
JensH0rnung:feat/api-audit-metadata

Conversation

@JensH0rnung

Copy link
Copy Markdown
Contributor

Feature:

Changes to a secret via the API now create an entry in the revision history of this secret. Before, entries were only created if changes were made in the browser.

Styling the revision history:

  • Separate icons for the change-type in their own column
  • Show both icons, if changes include secret_data AND metadata
  • Add fixed table-layout with percentage column widths
  • Truncate long usernames with tooltip showing the full name

Comment on lines +54 to +55
current_data = instance.current_revision.peek_data(self.request.user)
RevisionService.save_payload(secret=instance, actor=self.request.user, payload=current_data)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this might be an issue for file secrets, peek_data returns raw bytes, but the payload parameter needs to be json-serializeable.

In the view, we guard it like this:

if secret.content_type == ContentType.FILE and isinstance(current_data, (bytes, bytearray)):
      current_data = {'file_content': base64.b64encode(current_data).decode()}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the guard:
cdc4ed3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! Though I still think there's an issue here, that I missed on my first review.
peek_data only returns the file contents; if the payload was stored with a file_name, this would erase that on metadata changes. I think the best way around this would be to make the payload optional in the RevisionService.save_payload(), and use secret.current_revision if it's None there.
That way, we don't need all this peek_data and the file-type guard, it just collapses to

elif instance.current_revision:
    RevisionService.save_payload(secret=instance, actor=self.request.user, payload=None)

We can then also do the same for the browser view.

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.

2 participants