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

Added plugin hooks before and after the kirby user gets logged in #34

Open
wants to merge 4 commits into
base: release-3.1.0
Choose a base branch
from

Conversation

Greakz
Copy link

@Greakz Greakz commented Mar 15, 2024

Added hooks before and after the kirby user gets logged in.
This was suggested in #30 (comment)

An example use-case is to process the given roles in the oAuthUserData to assign kirby roles.
It would be nice if something like this could be added to the plugin. Feel free to change anything.

@thathoff
Copy link
Owner

Thanks for your pull request. I would prefer to have the second parameter of the hook require a ResourceOwnerInterface as this is typed and could make things more flexible (so in code it would be $oauthUser).

I would also move the before hook before the whole gate logic starting here so it's possible to prevent user creation with the hook.

@thathoff thathoff self-assigned this Mar 17, 2024
@Greakz
Copy link
Author

Greakz commented Mar 18, 2024

So, for my understanding:
Instead of the oauthUserData you want to pass the oauthUser to provide the full ResourceOwnerInterface. This would change the first parameter of the hook. The second is the kirby user, so that an additional plugin can work with the oauthUser and the kirby-user.

You suggest the before hook in front of line 148 to make it possible to prevent user creation? Or do we need to put that hook inside the If-clause to only trigger when the user does not exist (in this case, before line 149)? This hook would not be a login:before-hook in my opinion because it's before the user-creation, so this should be a third hook with only the oauthUser I think.

Something like: $this->kirby->trigger('thathoff.oauth.user-creation:before', ['oauthUser' => $oauthUser]);

Do you agree, or did I misunderstand something?

@thathoff
Copy link
Owner

thathoff commented Apr 4, 2024

Sorry for my late reply, I’ve been on vacation.

Instead of the oauthUserData you want to pass the oauthUser to provide the full ResourceOwnerInterface. This would change the first parameter of the hook. The second is the kirby user, so that an additional plugin can work with the oauthUser and the kirby-user.

Yes you’re right.

You suggest the before hook in front of line 148 to make it possible to prevent user creation? Or do we need to put that hook inside the If-clause to only trigger when the user does not exist (in this case, before line 149)? This hook would not be a login:before-hook in my opinion because it's before the user-creation, so this should be a third hook with only the oauthUser I think.

Then i would go with:

$createResult = $this->kirby->trigger('thathoff.oauth.user-create:before', ['oauthUser' => $oauthUser]);
$kirbyUser = null;

// if the hook returns a kirby user we take this one
if ($createResult instanceof \Kirby\Cms\User) {
   $kirbyUser = $createResult
}

// if the hook returns null or true the user is created
if ($createResult === true || $createResult === null) {
   // go on with user creation
}

if (!$kirbyUser) {
    $this->error("User cannot be created.");
}

and

$this->kirby->trigger('thathoff.oauth.user-create:after', ['oauthUser' => $oauthUser, 'user' => $kirbyUser]);

@pReya
Copy link
Contributor

pReya commented Apr 14, 2024

@Greakz Will you have time to implement the changes suggested by @thathoff ?

Luke Thienemann added 2 commits April 15, 2024 09:58
…write or stop the user creation. Added thathoff.oauth.user-create:after-hook
@Greakz
Copy link
Author

Greakz commented Apr 15, 2024

Sorry for my late reply, I’ve been on vacation.

Same here, sorry.

@Greakz Will you have time to implement the changes suggested by @thathoff ?

Yes, the changes should be implemented. The only thing left to do is change the version in composer.json (I guess this will be 3.1.0?) but i think this is up to you @thathoff

Tell me if you want other changes in this PullRequest or if you are fine with what we got now @thathoff

@thathoff thathoff changed the base branch from master to release-3.1.0 April 15, 2024 08:41
@thathoff
Copy link
Owner

Thank you! I’ll check everything and maybe include this feature right into the next release.

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

Successfully merging this pull request may close these issues.

3 participants