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.

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

Add ImpersonateEvents for SnappyMail (and others) #180

Merged
merged 5 commits into from
Feb 28, 2023

Conversation

the-djmaze
Copy link

More info at #179

@blizzz
Copy link
Member

blizzz commented Nov 4, 2022

Does it carry enough information to know which user is impersonated, and when impersonation finishes? It looks like to do guess work. What about having ImpersonateBeginEvent and ImeprsonateEndEvent, and have methods named more clearly like getImpersonatedUser and getImpersonatingUser?

@the-djmaze
Copy link
Author

Does it carry enough information to know which user is impersonated

For me personally i don't care which user. Just "impersonate" is enough for me to force logout.

I've just extended it a bit if other apps also want to use it.
Yours would even be better for other apps that might want to know details regarding the impersonating.

Just do what you think is right.

@blizzz
Copy link
Member

blizzz commented Nov 8, 2022

@the-djmaze added a commit, does this work for you? I removed the legacy emitter, it should not be used anymore. It carries only some legacy bits (which should be phased out…)

djmaze and others added 3 commits November 8, 2022 12:45
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz changed the title Add ImpersonateEvent for SnappyMail (and others) Add ImpersonateEvents for SnappyMail (and others) Nov 8, 2022
@blizzz
Copy link
Member

blizzz commented Nov 15, 2022

@the-djmaze does it work for you? ^

@the-djmaze
Copy link
Author

I will check!

@blizzz blizzz requested review from nickvergessen, come-nc, a team, icewind1991 and CarlSchwan and removed request for a team December 2, 2022 22:08
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Fine in general
Just two suggestions

$user = $this->session->get('oldUserId');
$user = $this->userManager->get($user);
$impersonator = $this->session->get('oldUserId');
$impersonator = $this->userManager->get($impersonator);
Copy link
Member

Choose a reason for hiding this comment

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

Would recommend different variable names for the uid and the user object

Copy link
Member

Choose a reason for hiding this comment

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

Had the same feeling and almost changed it back then. Well, almost. 👍

@@ -60,7 +66,10 @@ public function logout(string $userId): JSONResponse {
);
}

$this->userSession->setUser($user);
$impersonatee = $this->userManager->get($userId);
Copy link
Member

Choose a reason for hiding this comment

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

I love the more clearer distinction on the name of the Getter function on the event.
...tee vs ...tor is visually so similar. $impersonatedUser would be more different.

Copy link
Member

Choose a reason for hiding this comment

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

Same gut feeling, 👍

Copy link
Member

Choose a reason for hiding this comment

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

Ah, actually as i did in the Event class… =D

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@the-djmaze
Copy link
Author

the-djmaze commented Dec 5, 2022

On logout i get:
Argument 2 passed to OCA\\Impersonate\\Events\\AImpersonateEvent::__construct() must implement interface OCP\\IUser, null given

Further it works as expected and helps a lot!

the-djmaze pushed a commit to the-djmaze/snappymail that referenced this pull request Dec 5, 2022
@blizzz
Copy link
Member

blizzz commented Dec 5, 2022

On logout i get: Argument 2 passed to OCA\\Impersonate\\Events\\AImpersonateEvent::__construct() must implement interface OCP\\IUser, null given

Further it works as expected and helps a lot!

I'll have another look, thanks for spotting it!

@blizzz
Copy link
Member

blizzz commented Feb 13, 2023

On logout i get: Argument 2 passed to OCA\\Impersonate\\Events\\AImpersonateEvent::__construct() must implement interface OCP\\IUser, null given
Further it works as expected and helps a lot!

I'll have another look, thanks for spotting it!

Probably needs adjustements on the impersonate_logout.js code, perhaps getting rid of jquery traces 🙈 needs more looking into

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
Copy link
Member

blizzz commented Feb 27, 2023

Latest commit fixes logout

@blizzz blizzz merged commit 798a3d5 into nextcloud:master Feb 28, 2023
@blizzz
Copy link
Member

blizzz commented Feb 28, 2023

/backport to stable25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants