p2-inc / keycloak-orgs

Single realm, multi-tenancy for SaaS apps
https://phasetwo.io
Other
389 stars 66 forks source link

Invite Email with necessary params missing #49

Closed mweibel closed 1 year ago

mweibel commented 1 year ago

Hi,

Just starting to try out keycloak-orgs (thanks for open sourcing this!) and stumbled upon the invite email sent having missing params:

michael@example.org has been invited to join Personal at MyRealm by . To accept or reject this invitation, click on the link below to log in.

That's the full email. No link is below and the inviter param is empty too even though the inviter has firstname/lastname and email. Any clue why that doesn't work?

FTR using keycloak-orgs at HEAD 80b4af5b1fd1a207ce5505c24eaf33b4568c80e0.

xgp commented 1 year ago

@mweibel thanks for the report. Can you tell me how you are initiating the email? Admin UI? API? In both cases, if you could send the request that is getting made, that would help debug.

mweibel commented 1 year ago

@xgp thanks for your reply!

Using the API:

~~~ REQUEST ~~~
POST  /auth/realms/MyRealm/orgs/d98aac19-a2e8-4a7a-b58f-dd40471717cb/invitations  HTTP/1.1
HOST   : keycloak:8080
HEADERS:
    Accept: application/json
    Authorization: Bearer REDACTED
    Content-Type: application/json
    User-Agent: go-resty/2.7.0 (https://github.com/go-resty/resty)
BODY   :
{
   "email": "test+foo@example.org",
   "send": true,
   "inviterId": "5aa3f978-1751-4296-9a0f-c10d49e6ddf5",
   "redirectUri": "http://localhost:8080",
   "roles": null
}
------------------------------------------------------------------------------
~~~ RESPONSE ~~~
STATUS       : 201 Created

Previously I didn't have the redirectUri set. Now with the redirectUri set it'll have the 4th param (the link) set but to exactly what I set there:

test+foo@example.org has been invited to join Personal at MyRealm by . To accept or reject this invitation, click on the link below to log in.

http://localhost:8080/

Shouldn't that be a link to keycloak which then after accepting the invitation, redirect back to the redirectUri?

xgp commented 1 year ago

Shouldn't that be a link to keycloak which then after accepting the invitation, redirect back to the redirectUri?

Ah. I see that this is confusing. Invitation acceptance is handled entirely as a Keycloak Required Action. That means we're making an assumption the redirect_uri you use is the place you want the user to go, and is also an application protected by Keycloak. That means when the user clicks on that link, he is redirected (by your application) to a Keycloak login flow, after a successful log in (or registration), the Required Action will trigger and prompt the user to accept/reject the organization's invitation. Following that required action, the user will be redirected to the original redirect_uri.

That's the way it works today, but let me know if you want/expect an alternative flow.

Also, I'll check out why the inviterId is getting ignored and not making it into the email. That one looks like a bug.

a8t3r commented 1 year ago

@xgp Alternative flow: when the user clicks on the link, he is not redirected to keycloak and handled by the application itself. I think it's strange when the invitation is sent from an organization, but the processing forces the user to select from a list of organizations. It would be nice to be able to process the link click on the backend side, and not on the keyclock. In other words, it should be possible to insert an invitation id into the redirect link.

This is currently not possible because id generation and sending an email is done in one step. A possible suggestion is to introduce additional update method and play with send attribute:

val request = InvitationRequest().apply {
  this.send = false
}
val invitation = createInvitation(request)
updateInvitation(invitation.apply {
  this.send = true
  this.redirectUri = "http://foobar.com/invitationId=${this.id}"
})

On the other hand, the invitation can be derived by a unique key organization_id, email, but I think this method is less convenient.

xgp commented 1 year ago

@a8t3r I like this suggestion.

  1. We've played with the invitation link being an action token (e.g. how we do magic links), which would add a required action specifically for that org's invite, and/or do a force authentication if the user is already logged in. This solves two of the current downsides with the current approach. One, that the invitation accept is currently generic, and that nothing actually happens if the user is already logged in.
  2. We've also thought about adding an API method that allows the automatic acceptance of an invitation, but we decided against that as the user isn't opting in to the invitation. We'll put some more thought this week and see what we come up with.
mweibel commented 1 year ago

Both options seem good to me, TBH. Also providing an API would be great since it's up to the application implementing it how to handle consent.

@xgp About the invitation flow, thanks for the reply. Did you find anything yet regarding the name?

Related to the invitation link, I tried the following now:

  1. Register user A via API
  2. Invite user B via API
  3. User B goes to http://localhost:18080/auth/realms/REDACTED/login-actions/registration?client_id=account-console&tab_id=REDACTED and registers

Expected behavior At this point, if I'm right, the invitation confirm should come and permit the user to accept the invite or not

Actual behavior The user is redirected to their account dashboard. No invitation confirm page is shown. When trying to log out and login again, no confirm is shown either.

Login flow of that realm image

Is there anything else I need to configure? Looking into logs I see the following:

2023-03-28 08:17:33,116 INFO  [io.phasetwo.keycloak.themes.theme.FreeMarkerAndMustacheEmailTemplateProviderFactory] (executor-thread-24) creating new FreeMarkerAndMustacheEmailTemplateProviderFactory
2023-03-28 08:17:33,116 INFO  [io.phasetwo.keycloak.themes.theme.FreeMarkerAndMustacheEmailTemplateProvider] (executor-thread-24) processTemplate for invitation-email.ftl in theme keycloak
2023-03-28 08:17:33,117 INFO  [io.phasetwo.keycloak.themes.theme.MustacheProvider] (executor-thread-24) templateType=null
2023-03-28 08:18:00,417 INFO  [io.phasetwo.service.auth.invitation.InvitationAuthenticator] (executor-thread-24) InvitationAuthenticator.configuredFor called for realm REDACTED and user asdf@example.org
2023-03-28 08:18:00,418 INFO  [io.phasetwo.service.auth.invitation.InvitationAuthenticator] (executor-thread-24) Found 1 invites for asdf@example.org
2023-03-28 08:18:00,418 INFO  [io.phasetwo.service.auth.invitation.InvitationAuthenticator] (executor-thread-24) InvitationAuthenticator.setRequiredActions called for realm REDACTED and user asdf@example.org
2023-03-28 08:18:00,435 WARN  [org.keycloak.services.managers.AuthenticationManager] (executor-thread-24) Could not find configuration for Required Action invitation-required-action, did you forget to register it?
a8t3r commented 1 year ago

@mweibel Make sure Realm -> Authentication -> Required Actions -> Invitation is enabled.

mweibel commented 1 year ago

@a8t3r oh! I missed that. Thanks a lot.

mweibel commented 1 year ago

@xgp I guess I found why the email doesn't have the name of the inviter:

https://github.com/p2-inc/keycloak-orgs/blob/c90bfb5dfc96853ab4c33e1e7ef0072ea6b03084/src/main/java/io/phasetwo/service/resource/InvitationsResource.java#L102

this uses the auth.getUser() (i.e. the access token owner) as the inviter instead of the user specified with inviterId.

a8t3r commented 1 year ago

I think it's not. The auth.getUser() is the subject who create the request and also the inviter. So auth.getUser().getId() should be equals to inviterId. Take a look at OrganizationAdapter.

I think, an error with your code at line:

String inviterName = getInviterName(inviter).orElse("");

Make sure inviter has firstname or lastname or email. If so, then this is an additional bug)

mweibel commented 1 year ago

@a8t3r what for is the separate inviterID param if the current logged in user is used?

We're calling the invite user using a client which acts on behalf of users (as an API). The user is, in this case, is probably the service account of the client? Maybe I'm doing something wrong or just different than what the extension expects. If you use the API, do you generate an impersonation token for the user who is the actor or how do you do it?

xgp commented 1 year ago

@mweibel @a8t3r The original intent of the inviterId param was to allow users with permission to act on behalf of other users. This allowed things like the "organization admin" account (basically a service account for orgs) to send an invitation of behalf of a user to another user. I think we just overlooked it in the implementation.

mweibel commented 1 year ago

@xgp that's what I thought too, yeah. See #55 for a fix for it.

Out of curiosity: how do you call the API in your case? Does the user itself call it via keycloak directly?

xgp commented 1 year ago

@mweibel We either have the user call it directly, or use the "organization admin" service account.