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

refactor(encryption): Migrate from hooks to events #48332

Merged
merged 2 commits into from
Oct 15, 2024
Merged

refactor(encryption): Migrate from hooks to events #48332

merged 2 commits into from
Oct 15, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Sep 25, 2024 •
edited
Loading

Summary

Migrate code from hooks to typed events, also unify the user events as some had already a getUid method -> now all provide this convenient helper.

Checklist

@susnux susnux added technical debt ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) labels Sep 25, 2024
@susnux susnux added this to the Nextcloud 31 milestone Sep 25, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 25, 2024
View reviewed changes
apps/encryption/lib/AppInfo/Application.php Fixed Show resolved Hide resolved
apps/encryption/lib/Listeners/UserEventsListener.php Fixed Show fixed Hide fixed
apps/encryption/lib/Listeners/UserEventsListener.php Fixed Show fixed Hide fixed
apps/encryption/lib/Listeners/UserEventsListener.php Fixed Show fixed Hide fixed
apps/encryption/lib/Listeners/UserEventsListener.php Fixed Show fixed Hide fixed
apps/encryption/lib/Services/PassphraseService.php Fixed Show fixed Hide fixed
apps/encryption/lib/Services/PassphraseService.php Fixed Show fixed Hide fixed
apps/encryption/lib/Services/PassphraseService.php Fixed Show fixed Hide fixed
apps/encryption/lib/Services/PassphraseService.php Fixed Show fixed Hide fixed
apps/encryption/lib/Services/PassphraseService.php Fixed Show fixed Hide fixed
@susnux susnux force-pushed the chore/migrate-encryption-away-from-hooks branch from f32d12e to dbf60ac Compare September 25, 2024 01:03
@susnux susnux force-pushed the chore/migrate-encryption-away-from-hooks branch from 93dea23 to 09afc2d Compare September 25, 2024 01:22
@susnux susnux force-pushed the chore/migrate-encryption-away-from-hooks branch 5 times, most recently from a080187 to e392348 Compare September 25, 2024 15:20
@susnux susnux added the 3. to review Waiting for reviews label Sep 25, 2024
provokateurin
provokateurin approved these changes Sep 25, 2024
View reviewed changes
apps/encryption/lib/KeyManager.php
Comment on lines +301 to +304
if ($passPhrase === null) {
$this->logger->warning('Master key is disabled but not passphrase provided.');
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a change not entirely related to the refactor and it should also be backported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a psalm error that was ignored before as it could be possible that null is passed (e.g. with master key) this was simply ignored before.

apps/encryption/lib/Listeners/UserEventsListener.php Outdated Show resolved Hide resolved
come-nc
come-nc reviewed Sep 26, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Not sure I like the getUid thing, especially since it uses a different casing than the method from IUser.
Otherwise, great work!

apps/encryption/lib/AppInfo/Application.php Show resolved Hide resolved
apps/encryption/lib/Listeners/UserEventsListener.php Outdated Show resolved Hide resolved
apps/encryption/lib/Services/PassphraseService.php Outdated Show resolved Hide resolved
core/Events/BeforePasswordResetEvent.php
@@ -34,6 +34,13 @@ public function getUser(): IUser {
return $this->user;
}

/**
* @since 31.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance of moving these events to OCP without creating a mess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC the event name is the class string of the event, meaning we can not move without also emitting the old event until it is deprecated enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I would say this makes sense, but please in a follow up

lib/private/Log/ExceptionSerializer.php Show resolved Hide resolved
@susnux
Copy link
Contributor Author

susnux commented Sep 26, 2024

Not sure I like the getUid thing, especially since it uses a different casing than the method from IUser.

The IUser one is quite old, older than our naming guidelines that prefer Uid over UID (or Url over URL).
But I am fine with removing it again, I just thought it would make sense to unify it as there are already events with getUid (exactly that casing).

@susnux susnux force-pushed the chore/migrate-encryption-away-from-hooks branch from c9eaac9 to fa428fb Compare September 26, 2024 18:14
@susnux susnux requested review from artonge and come-nc September 26, 2024 18:16
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 26, 2024
View reviewed changes
apps/encryption/lib/Listeners/UserEventsListener.php Outdated
$this->onPasswordUpdated($event->getUid(), $event->getPassword(), $event->getRecoveryPassword());
} elseif($event instanceof BeforePasswordResetEvent) {
$this->onBeforePasswordReset($event->getUid());
} elseif($event instanceof PasswordResetEvent) {

Check notice

Code scanning / Psalm

RedundantConditionGivenDocblockType

Docblock-defined type OC\Core\Events\PasswordResetEvent for $event is always OC\Core\Events\PasswordResetEvent
come-nc
come-nc reviewed Sep 30, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

All good apart from the catch Throwable thing.

@susnux susnux force-pushed the chore/migrate-encryption-away-from-hooks branch from 6654d12 to 4582d30 Compare October 11, 2024 12:20
@susnux susnux requested a review from come-nc October 11, 2024 12:20
@susnux susnux force-pushed the chore/migrate-encryption-away-from-hooks branch 3 times, most recently from 0be66d1 to bcd59f4 Compare October 15, 2024 15:06
susnux and others added 2 commits October 15, 2024 18:33
@susnux @artonge @come-nc
refactor(encryption): Migrate away from Hooks to typed events
f3aa004 
Co-authored-by: Ferdinand Thiessen <[email protected]>
Co-authored-by: Louis <[email protected]>
Co-authored-by: Côme Chilliet <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux
chore: Unify user related events to always provide a getUid method
b4ec7ca 
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the chore/migrate-encryption-away-from-hooks branch from bcd59f4 to b4ec7ca Compare October 15, 2024 16:33
@susnux susnux merged commit 5b007b7 into master Oct 15, 2024
177 checks passed
@susnux susnux deleted the chore/migrate-encryption-away-from-hooks branch October 15, 2024 17:17
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc approved these changes

@provokateurin provokateurin provokateurin approved these changes

@artonge artonge Awaiting requested review from artonge

Assignees

@susnux susnux

Labels
3. to review Waiting for reviews ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) technical debt
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.
4 participants
@susnux @artonge @provokateurin @come-nc