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.

Sign up for GitHub

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

Jump to bottom

Mark database fields as sensitive so they dont show up in logs #4720

Merged
merged 9 commits into from
May 17, 2024
Merged

Mark database fields as sensitive so they dont show up in logs #4720

merged 9 commits into from
May 17, 2024

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented May 15, 2024

No description provided.

Nutomic
Nutomic commented May 15, 2024
View reviewed changes
crates/db_schema/src/sensitive.rs
#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize, Default)]
#[cfg_attr(feature = "full", derive(DieselNewType))]
#[serde(transparent)]
pub struct SensitiveString(String);
Copy link
Member Author

Choose a reason for hiding this comment

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

The generic here was completely unnecessary as it was only used for strings. Also a few of the impls were unused so I removed them.

Copy link
Member

Choose a reason for hiding this comment

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

Seems fine.

dessalines
dessalines approved these changes May 15, 2024
View reviewed changes
Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

I wonder why it was previously in api_common, rather than the DB crate, but nothing looked missing in the diff.

crates/db_schema/src/sensitive.rs
#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize, Default)]
#[cfg_attr(feature = "full", derive(DieselNewType))]
#[serde(transparent)]
pub struct SensitiveString(String);
Copy link
Member

Choose a reason for hiding this comment

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

Seems fine.

@dessalines
Copy link
Member

Clippy is failing, probably missing an into_inner()

SleeplessOne1917
SleeplessOne1917 reviewed May 15, 2024
View reviewed changes
crates/apub/src/objects/community.rs Outdated Show resolved Hide resolved
crates/apub/src/objects/instance.rs Outdated Show resolved Hide resolved
crates/apub/src/objects/person.rs Outdated Show resolved Hide resolved
dessalines and others added 4 commits May 16, 2024 16:42
@dessalines @SleeplessOne1917
Update crates/apub/src/objects/person.rs
8a62fb4 
Co-authored-by: SleeplessOne1917 <28871516+SleeplessOne1917@users.noreply.github.com>
@dessalines @SleeplessOne1917
Update crates/apub/src/objects/community.rs
a28dc3f 
Co-authored-by: SleeplessOne1917 <28871516+SleeplessOne1917@users.noreply.github.com>
@dessalines @SleeplessOne1917
Update crates/apub/src/objects/instance.rs
636fa33 
Co-authored-by: SleeplessOne1917 <28871516+SleeplessOne1917@users.noreply.github.com>
@dessalines
Merge branch 'main' into db-fields-sensitive
e8af6f8
@dessalines dessalines enabled auto-merge (squash) May 16, 2024 20:43
@SleeplessOne1917
Copy link
Member

Looks like I made a mistake with my clippy fix suggestions. It turns out the SensitiveString type isn't being imported in the files I commented on:

error[E0433]: failed to resolve: use of undeclared type `SensitiveString`
   --> crates/apub/src/objects/person.rs:203:34
    |
203 |     self.private_key.clone().map(SensitiveString::into_inner)
    |                                  ^^^^^^^^^^^^^^^ use of undeclared type `SensitiveString`
    |
help: consider importing this struct
    |
1   + use lemmy_db_schema::sensitive::SensitiveString;
    |

@dessalines
Copy link
Member

No probs, I'll get.

@dessalines dessalines merged commit 99aac07 into main May 17, 2024
2 checks passed
@SleeplessOne1917 SleeplessOne1917 deleted the db-fields-sensitive branch May 17, 2024 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dullbananas dullbananas dullbananas approved these changes

@SleeplessOne1917 SleeplessOne1917 SleeplessOne1917 approved these changes

@dessalines dessalines dessalines approved these changes

@phiresky phiresky Awaiting requested review from phiresky phiresky is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Nutomic @dessalines @SleeplessOne1917 @dullbananas