Skip to content

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

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
Merged
susnux merged 2 commits into from
Oct 15, 2024
Merged

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

susnux 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
@susnux susnux force-pushed the chore/migrate-encryption-away-from-hooks branch from f32d12e to dbf60ac Compare September 25, 2024 01:03
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 25, 2024
View reviewed changes
@susnux susnux force-pushed the chore/migrate-encryption-away-from-hooks branch from 93dea23 to 09afc2d Compare September 25, 2024 01:22
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 25, 2024
View reviewed changes
@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

@provokateurin provokateurin Sep 25, 2024

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

@susnux susnux Sep 25, 2024

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.

artonge
artonge reviewed Sep 26, 2024
View reviewed changes
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!

core/Events/BeforePasswordResetEvent.php
}

/**
* @since 31.0.0
Copy link
Contributor

@come-nc come-nc Sep 26, 2024

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

@susnux susnux Sep 26, 2024

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

@susnux susnux Sep 26, 2024

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

@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
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.

come-nc
come-nc reviewed Sep 30, 2024
View reviewed changes
@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
@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
@joshtrichards joshtrichards mentioned this pull request Mar 6, 2025
Closed
8 tasks
@susnux susnux mentioned this pull request Mar 6, 2025
Merged
4 tasks
@come-nc come-nc mentioned this pull request Jun 16, 2025
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@provokateurin provokateurin provokateurin approved these changes

@come-nc come-nc come-nc 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.

5 participants

@susnux @artonge @provokateurin @come-nc