Skip to content

Add support for archiving items#6916

Open
matt-aaron wants to merge 2 commits intodani-garcia:mainfrom
matt-aaron:archiving
Open

Add support for archiving items#6916
matt-aaron wants to merge 2 commits intodani-garcia:mainfrom
matt-aaron:archiving

Conversation

@matt-aaron
Copy link
Copy Markdown

Adds support for archiving items: #6372

I'm new to the codebase, so let me know if I have missed anything. Happy to apply any feedback you may have. Thank you!

Comment thread src/api/core/ciphers.rs
@matt-aaron matt-aaron requested a review from BlackDex March 9, 2026 12:20
Comment thread src/config.rs Outdated
"anon-addy-self-host-alias",
"simple-login-self-host-alias",
"mutual-tls",
"pm-19148-innovation-archive",
Copy link
Copy Markdown
Contributor

@stefan0xC stefan0xC Mar 9, 2026

Choose a reason for hiding this comment

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

@BlackDex I have not tested it (and I am not sure how stable this feature is yet) but could we not just enable this feature directly instead of adding a new feature flag that you have to opt in?

Also the feature flags we set have already been removed for a few releases, so they can be safely replaced in my opinion.

feature_states.insert("duo-redirect".to_string(), true);
feature_states.insert("email-verification".to_string(), true);
feature_states.insert("unauth-ui-refresh".to_string(), true);
feature_states.insert("enable-pm-flight-recorder".to_string(), true);
feature_states.insert("mobile-error-reporting".to_string(), true);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

With it being included in the 2026.2.1 release announcement, I think it'd be stable enough to enable directly

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have enabled the flag directly and removed some of the older flags (keeping email-verification and mobile-error-reporting).

@BlackDex Let me know if this needs further adjustments

@qianlongzt
Copy link
Copy Markdown

I’m just wondering why we don’t simply add a field to the ciphers table.

https://github.com/bitwarden/server/blob/main/src/Sql/dbo/Vault/Tables/Cipher.sql#L16
ref: bitwarden/server#6578

@matt-aaron
Copy link
Copy Markdown
Author

matt-aaron commented Mar 9, 2026

I’m just wondering why we don’t simply add a field to the ciphers table.

https://github.com/bitwarden/server/blob/main/src/Sql/dbo/Vault/Tables/Cipher.sql#L16 ref: bitwarden/server#6578

They originally did that, but later added the archives column to that table. This enables users the archives to be per-user instead of impacting other users within an org, for example.

@matt-aaron matt-aaron force-pushed the archiving branch 2 times, most recently from f5497c3 to 4bfc0b2 Compare March 9, 2026 18:03
@matt-aaron
Copy link
Copy Markdown
Author

Refactored to properly support importing items that were archived

@matt-aaron matt-aaron force-pushed the archiving branch 2 times, most recently from ba40076 to 6fc9aa0 Compare March 24, 2026 03:16
@dcelasun
Copy link
Copy Markdown

dcelasun commented Apr 2, 2026

@BlackDex any further thoughts on this? Seems fairly straightforward and previous comments were addressed.

@matt-aaron matt-aaron force-pushed the archiving branch 2 times, most recently from 6ac5beb to 4a73d26 Compare April 2, 2026 14:07
@BlackDex
Copy link
Copy Markdown
Collaborator

BlackDex commented Apr 2, 2026

I need time to check and verify this, or @dani-garcia must have some time of course.
But I am busy with a lot of other stuff which needs attention, and for me, new feature are nice, especially this one, but they do come in a lower rank then some other items.

Comment thread src/config.rs Outdated
// Platform Team
"pm-30529-webauthn-related-origins",
// Innovation Team
"pm-19148-innovation-archive",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is not needed when adding it as a default flag, since those can't be adjusted by users anyway.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah, true. Removed

Comment thread src/db/schema.rs
archived_at -> Timestamp,
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looking at the migration sql's i think there are some joinable!() lines missing between the archives and users/ciphers tables. Without those Diesel doesn't treat those as joinable records during queries or might be unable to optimize the queries.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added the joinable!() lines and also included archives in the allow_tables_to_appear_in_same_query!() macro

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.

5 participants