Skip to content

Move admin_audit to proper event listeners v2 #47865

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
come-nc merged 8 commits into from
Sep 13, 2024
Merged

Move admin_audit to proper event listeners v2 #47865

come-nc merged 8 commits into from
Sep 13, 2024

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Sep 10, 2024 •
edited
Loading

See #32128

Summary

Rebased version of #37193
Part of original PR were already done in master so some commit endup a bit empty, but everything should work.

Does not fix all cases of deprecated mecanism but this is already a good improvement and as such it should be merged before it conflicts again.

Checklist

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 10, 2024
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@come-nc come-nc changed the title Admin audit/enh/move to event listeners v2 Move admin_audit to proper event listeners v2 Sep 10, 2024
@come-nc come-nc self-assigned this Sep 10, 2024
@come-nc come-nc added this to the Nextcloud 31 milestone Sep 10, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 10, 2024
View reviewed changes
apps/admin_audit/lib/Listener/AppManagementEventListener.php
$this->appEnable($event);
} elseif ($event instanceof AppDisableEvent) {
$this->appDisable($event);
} elseif ($event instanceof AppUpdateEvent) {

Check notice

Code scanning / Psalm

RedundantConditionGivenDocblockType

Docblock-defined type OCP\App\Events\AppUpdateEvent for $event is always OCP\App\Events\AppUpdateEvent
apps/admin_audit/lib/Listener/AuthEventListener.php
$this->beforeUserLoggedIn($event);
} elseif ($event instanceof UserLoggedInWithCookieEvent || $event instanceof UserLoggedInEvent) {
$this->userLoggedIn($event);
} elseif ($event instanceof BeforeUserLoggedOutEvent) {

Check notice

Code scanning / Psalm

RedundantConditionGivenDocblockType

Docblock-defined type OCP\User\Events\BeforeUserLoggedOutEvent for $event is always OCP\User\Events\BeforeUserLoggedOutEvent
apps/admin_audit/lib/Listener/ConsoleEventListener.php
*/
class ConsoleEventListener extends Action implements IEventListener {
public function handle(Event $event): void {
if ($event instanceof ConsoleEvent) {

Check notice

Code scanning / Psalm

RedundantConditionGivenDocblockType

Docblock-defined type OCP\Console\ConsoleEvent for $event is always OCP\Console\ConsoleEvent
apps/admin_audit/lib/Listener/UserManagementEventListener.php
$this->passwordUpdated($event);
} elseif ($event instanceof UserIdAssignedEvent) {
$this->userIdAssigned($event);
} elseif ($event instanceof UserIdUnassignedEvent) {

Check notice

Code scanning / Psalm

RedundantConditionGivenDocblockType

Docblock-defined type OCP\User\Events\UserIdUnassignedEvent for $event is always OCP\User\Events\UserIdUnassignedEvent
apps/user_ldap/lib/Access.php
} elseif (!$isUser) {
$this->dispatcher->dispatchTyped(new UserIdAssignedEvent($name));
if ($this->ncUserManager instanceof PublicEmitter) {
$this->ncUserManager->emit('\OC\User', 'assignedUserId', [$name]);

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OC\Hooks\PublicEmitter::emit has been marked as deprecated
apps/user_ldap/lib/Jobs/Sync.php
parent::__construct($timeFactory);
$this->setInterval(
(int)\OC::$server->getConfig()->getAppValue(
(int)$this->config->getAppValue(

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCP\IConfig::getAppValue has been marked as deprecated
apps/user_ldap/lib/Jobs/Sync.php

$cycleData = [
'prefix' => $this->config->getAppValue('user_ldap', 'background_sync_prefix', null),
'prefix' => $this->config->getAppValue('user_ldap', 'background_sync_prefix', 'none'),

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCP\IConfig::getAppValue has been marked as deprecated
apps/user_ldap/lib/Jobs/Sync.php
public function setCycle(array $cycleData): void {
$this->config->setAppValue('user_ldap', 'background_sync_prefix', $cycleData['prefix']);
$this->config->setAppValue('user_ldap', 'background_sync_offset', $cycleData['offset']);
$this->config->setAppValue('user_ldap', 'background_sync_offset', (string)$cycleData['offset']);

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCP\IConfig::setAppValue has been marked as deprecated
@come-nc come-nc force-pushed the admin_audit/enh/move-to-event-listeners-v2 branch from f52dda0 to ec37338 Compare September 10, 2024 12:06
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 10, 2024
@come-nc come-nc marked this pull request as ready for review September 10, 2024 12:32
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 10, 2024
View reviewed changes
apps/admin_audit/lib/Listener/FileEventListener.php
*/
class FileEventListener extends Action implements IEventListener {
public function handle(Event $event): void {
if ($event instanceof BeforePreviewFetchedEvent) {

Check notice

Code scanning / Psalm

RedundantConditionGivenDocblockType

Docblock-defined type OCP\Preview\BeforePreviewFetchedEvent for $event is always OCP\Preview\BeforePreviewFetchedEvent
@come-nc come-nc force-pushed the admin_audit/enh/move-to-event-listeners-v2 branch from 01f9bb4 to 957ac81 Compare September 10, 2024 12:48
@come-nc come-nc added the pending documentation This pull request needs an associated documentation update label Sep 10, 2024
kesselb
kesselb approved these changes Sep 10, 2024
View reviewed changes
apps/admin_audit/lib/Listener/SecurityEventListener.php
public function handle(Event $event): void {
if ($event instanceof TwoFactorProviderChallengePassed) {
$this->twoFactorProviderChallengePassed($event);
} elseif ($event instanceof TwoFactorProviderChallengeFailed) {
Copy link
Contributor

@kesselb kesselb Sep 10, 2024

Choose a reason for hiding this comment

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

I guess it's safe to ignore the psalm warning.
If you want to get rid of it, add "Event" to template-implements.

@come-nc come-nc requested review from a team, artonge and icewind1991 and removed request for a team September 10, 2024 14:45
@come-nc come-nc requested a review from nfebe September 10, 2024 14:45
miaulalala
miaulalala approved these changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

Some nitpicks you could implement but don't have to. Otherwise LGTM.

@come-nc @miaulalala
fix: Fix phpdoc wording
d767455 
Co-authored-by: Anna <[email protected]>
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 13, 2024
@come-nc come-nc enabled auto-merge September 13, 2024 14:08
@come-nc come-nc merged commit dfa994e into master Sep 13, 2024
@come-nc come-nc deleted the admin_audit/enh/move-to-event-listeners-v2 branch September 13, 2024 14:21
This was referenced Sep 13, 2024
Closed
Open
@kesselb kesselb mentioned this pull request Sep 16, 2024
Merged
7 tasks
@joshtrichards joshtrichards mentioned this pull request Sep 18, 2024
Closed
8 tasks
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Merged
@joshtrichards joshtrichards mentioned this pull request Feb 7, 2025
Merged
2 tasks
@susnux susnux mentioned this pull request Mar 12, 2025
Closed
@provokateurin provokateurin mentioned this pull request Mar 25, 2025
Merged
@come-nc come-nc mentioned this pull request Apr 3, 2025
Merged
@come-nc come-nc removed the pending documentation This pull request needs an associated documentation update label Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kesselb kesselb kesselb approved these changes

@miaulalala miaulalala miaulalala approved these changes

@susnux susnux Awaiting requested review from susnux

@tcitworld tcitworld Awaiting requested review from tcitworld

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 is a code owner automatically assigned from nextcloud/server-backend

@artonge artonge Awaiting requested review from artonge artonge is a code owner automatically assigned from nextcloud/server-backend

@nfebe nfebe Awaiting requested review from nfebe nfebe is a code owner automatically assigned from nextcloud/server-backend

Assignees

@come-nc come-nc

Labels

4. to release Ready to be released and/or waiting for tests to finish technical debt

Projects

None yet

Milestone

Nextcloud 31

Development

Successfully merging this pull request may close these issues.

6 participants

@come-nc @kesselb @miaulalala @tcitworld @CarlSchwan