Repository navigation
feat(workflows): add invite-user step - #49301
Conversation
|
External links check fix - #49291 |
a943f9f to
71867cb
Compare
46f6a29 to
8a60e84
Compare
There was a problem hiding this comment.
Nice contribution - the ActionTokenEmail utility and the DefaultActionToken.serialize(URI) refactoring are well done, and the step fills a practical gap for automated onboarding workflows. A few items to address before this can be merged:
1. Rebase needed - WorkflowsResource.create() on main now uses ErrorResponse.error(session, locale, me, ...) with locale support. The PR is based on an older version using ErrorResponse.error(me.getMessage(), ...). The new ComponentValidationException catch should also use the locale-aware variant.
2. Update path - WorkflowResource.update() also needs to catch ComponentValidationException. When there are no scheduled steps, updateWorkflow() deletes and re-creates the workflow via toModel(), which goes through realm.addComponentModel() and triggers validateConfiguration(). Even the scheduled-steps path calls workflow.updateConfig() which can trigger realm.updateComponent() - and that also calls validateConfiguration().
8a60e84 to
e8f22a4
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new invite-user workflow step that sends invitation emails with action-token links on user creation, along with supporting infrastructure and validation/error handling.
Changes:
- Introduces
InviteUserStepProvider/ factory and registers it as a workflow step provider. - Adds
ActionTokenEmailhelper and extends action-token serialization to support non-HTTP contexts (base URI). - Adds integration tests, server test config, admin API error mapping for step configuration validation, and documentation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/base/src/test/java/org/keycloak/tests/workflow/step/InviteUserStepTest.java | Integration tests for invite-user workflow behavior and token contents. |
| tests/base/src/test/java/org/keycloak/tests/workflow/config/InviteUserStepServerConfig.java | Test server config to provide a static hostname/base URL for invite links. |
| services/src/main/resources/META-INF/services/org.keycloak.models.workflow.WorkflowStepProviderFactory | Registers InviteUserStepProviderFactory for discovery. |
| services/src/main/java/org/keycloak/workflow/admin/resource/WorkflowsResource.java | Maps ComponentValidationException to HTTP 400 instead of 500 on create. |
| services/src/main/java/org/keycloak/workflow/admin/resource/WorkflowResource.java | Maps ComponentValidationException to HTTP 400 instead of 500 on update. |
| services/src/main/java/org/keycloak/models/workflow/InviteUserStepProviderFactory.java | Adds config validation for invite-user step. |
| services/src/main/java/org/keycloak/models/workflow/InviteUserStepProvider.java | Implements invite-user step runtime logic and base-URI resolution for background execution. |
| services/src/main/java/org/keycloak/email/ActionTokenEmail.java | Adds reusable helper for execute-actions emails (resolve + send). |
| services/src/main/java/org/keycloak/authentication/actiontoken/DefaultActionToken.java | Adds serialize(..., URI baseUri) overload for non-request contexts. |
| docs/documentation/server_admin/topics/workflows/defining-steps.adoc | Documents the new invite-user step and its configuration requirements. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e8f22a4 to
d0bb484
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
tests/base/src/test/java/org/keycloak/tests/workflow/config/InviteUserStepServerConfig.java:31
- This test config sets
hostnameto a full URL. If the runtime hostname v2 configuration expects full URLs viahostname-url, this setup can diverge from real deployments and may mask issues inresolveBaseUri. Consider switching the test to configure the full URL using the dedicated option (e.g.hostname-url) so the integration test exercises the intended configuration path.
public KeycloakServerConfigBuilder configure(KeycloakServerConfigBuilder config) {
return config
.option("spi-workflow--default--executor-blocking", Boolean.TRUE.toString())
.option("hostname", HOSTNAME_URL);
}
services/src/main/java/org/keycloak/models/workflow/InviteUserStepProvider.java:106
- Using
getOrDefaultwill not fall back toDEFAULT_ACTIONSwhen theactionskey exists but is configured as an empty list; that would send an execute-actions token with no required actions. Consider normalizing empty-to-default here (or rejecting empty lists invalidateConfiguration) to avoid silently sending invitations that do nothing.
List<String> actions = stepModel.getConfig().getOrDefault(CONFIG_ACTIONS, DEFAULT_ACTIONS);
try {
ActionTokenEmail.send(session, realm, user, resolved.getParams().get(), actions, baseUri);
tests/base/src/test/java/org/keycloak/tests/workflow/step/InviteUserStepTest.java:68
- Multiple tests in this class create workflows with the same name (e.g. "invite" / "invite-bad"). If the underlying test realm is shared across methods (common in integration tests), workflow-name uniqueness constraints can make the suite order-dependent or flaky. Consider using unique workflow names per test (e.g. suffix with the test method name/UUID) or deleting the workflow during teardown.
managedRealm.admin().workflows().create(WorkflowRepresentation.withName("invite")
.onEvent(UserCreatedWorkflowEventFactory.ID)
.withSteps(
WorkflowStepRepresentation.create().of(InviteUserStepProviderFactory.ID).build()
).build()).close();
docs/documentation/server_admin/topics/workflows/defining-steps.adoc:101
- The docs state that
--hostnamecan be set to a full URL and be used for invitation link construction. If hostname v2 uses a dedicated full-URL option (commonly--hostname-url) and treats--hostnameas host-only, this guidance may be misleading. Consider documenting the full-URL option explicitly (and keeping it consistent withInviteUserStepProvider.resolveBaseUri’s lookup order).
The `invite-user` step builds invitation links from the configured hostname, resolved from the realm `frontendUrl` attribute, the `KC_HOSTNAME_URL` environment variable, or the `--hostname` option when it is set to a full URL. Because the step runs on a background thread without an active request, one of these must be configured: a workflow containing an `invite-user` step is rejected at creation time with a `400 Bad Request` if none is set. The step is only skipped at runtime if the hostname configuration is removed after the workflow was created.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
tests/base/src/test/java/org/keycloak/tests/workflow/step/InviteUserStepTest.java:94
- This test asserts ‘no email was sent’ without waiting for the workflow executor to run. That can produce false positives if email sending is delayed (the assertion runs before the email is produced). Consider adding a short bounded wait (or an explicit ‘wait for no mail’ helper/poll) before asserting that no message exists.
@Test
public void testInviteSkippedForUserWithoutEmail() {
managedRealm.admin().workflows().create(WorkflowRepresentation.withName("invite")
.onEvent(UserCreatedWorkflowEventFactory.ID)
.withSteps(
WorkflowStepRepresentation.create().of(InviteUserStepProviderFactory.ID).build()
).build()).close();
managedRealm.admin().users().create(UserBuilder.create().username("noemail").build()).close();
assertNull(findEmailByRecipientContaining(mailServer, "noemail"));
}
tests/base/src/test/java/org/keycloak/tests/workflow/step/InviteUserStepTest.java:163
- Same issue as
testInviteSkippedForUserWithoutEmail(): this can be racy because it asserts immediately. Add a bounded wait/poll before asserting absence to avoid flaky false positives when the executor runs asynchronously.
@Test
public void testInviteSkippedForDisabledUser() {
managedRealm.admin().workflows().create(WorkflowRepresentation.withName("invite")
.onEvent(UserCreatedWorkflowEventFactory.ID)
.withSteps(
WorkflowStepRepresentation.create().of(InviteUserStepProviderFactory.ID).build()
).build()).close();
managedRealm.admin().users().create(UserBuilder.create()
.username("disabled").email("disabled@example.com").enabled(false).build()).close();
assertNull(findEmailByRecipientContaining(mailServer, "disabled@example.com"));
}
services/src/main/java/org/keycloak/models/workflow/InviteUserStepProviderFactory.java:84
- The PR introduces a new validation rule that rejects workflow creation when no static hostname is configured. There should be a test that asserts creating/updating a workflow with an
invite-userstep fails with400 Bad Requestwhen hostname is not configured (and ideally that it succeeds whenfrontendUrlor hostname is configured).
// The step runs on a workflow executor thread with no active HTTP request, so the
// invitation base URI must come from static configuration (see resolveBaseUri).
// Reject the step up-front instead of silently dropping invites at runtime.
if (InviteUserStepProvider.resolveBaseUri(session, realm) == null) {
throw new ComponentValidationException(InviteUserStepProvider.HOSTNAME_NOT_CONFIGURED_MESSAGE);
}
services/src/main/java/org/keycloak/workflow/admin/resource/WorkflowResource.java:109
- The
ComponentValidationException→400mapping logic is duplicated here and inWorkflowsResource#create(). Consider extracting a small shared helper (e.g., a private method in a common base resource or a utility onErrorResponse) to keep the mapping consistent and reduce the chance of future divergence.
} catch (ComponentValidationException cve) {
// updateWorkflow() re-creates the workflow (no scheduled steps) or calls
// updateConfig(), both of which run WorkflowStepProviderFactory.validateConfiguration()
// via realm.addComponentModel()/updateComponent() - report invalid step
// configuration as a client error instead of leaking a 500.
throw ErrorResponse.error(
ErrorResponse.resolveMessage(session, locale, cve.getMessage(), cve.getParameters()),
Response.Status.BAD_REQUEST);
}
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.federation.ldap.UserFederationLdapConnectionTest#Keycloak CI - FIPS IT (strict) |
|
@sguilhen Sorry for the delay, and thanks again for the review. All your points are addressed in the latest push, and I've also handled the Copilot round. On CI: the red jobs aren't introduced by this PR — they're pre-existing/flaky and also fail on current
A re-run should clear the flaky ones. Happy to rebase again if needed. |
There was a problem hiding this comment.
Thanks @bilkoua , you've addressed all my previous point. Only one thing now called my attention about the KC_HOSTNAME_URL usage, apart from that everything seems good.
Faliing CI tests are unrelated, a new run should clear the errors.
Closes keycloak#49297 Signed-off-by: bilkoua <git@bil.co.ua>
d0bb484 to
a67fa86
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (7)
tests/base/src/test/java/org/keycloak/tests/workflow/step/InviteUserStepTest.java:68
- These tests reuse the same workflow name (
invite) and client id (invite-client) across multiple test methods. If the realm/test container state is not fully reset between tests, this can cause name collisions (e.g., 409/conflict) and flaky behavior. Consider generating unique names per test (e.g., suffix with the test method name/UUID) or explicitly deleting created workflows/clients in teardown.
managedRealm.admin().workflows().create(WorkflowRepresentation.withName("invite")
.onEvent(UserCreatedWorkflowEventFactory.ID)
.withSteps(
WorkflowStepRepresentation.create().of(InviteUserStepProviderFactory.ID).build()
).build()).close();
tests/base/src/test/java/org/keycloak/tests/workflow/step/InviteUserStepTest.java:137
- These tests reuse the same workflow name (
invite) and client id (invite-client) across multiple test methods. If the realm/test container state is not fully reset between tests, this can cause name collisions (e.g., 409/conflict) and flaky behavior. Consider generating unique names per test (e.g., suffix with the test method name/UUID) or explicitly deleting created workflows/clients in teardown.
createClient("invite-client", "https://app.example.com/*");
tests/base/src/test/java/org/keycloak/tests/workflow/step/InviteUserStepTest.java:176
- These tests reuse the same workflow name (
invite) and client id (invite-client) across multiple test methods. If the realm/test container state is not fully reset between tests, this can cause name collisions (e.g., 409/conflict) and flaky behavior. Consider generating unique names per test (e.g., suffix with the test method name/UUID) or explicitly deleting created workflows/clients in teardown.
managedRealm.admin().workflows().create(WorkflowRepresentation.withName("invite")
.onEvent(UserCreatedWorkflowEventFactory.ID)
.withSteps(
WorkflowStepRepresentation.create().of(InviteUserStepProviderFactory.ID)
.withConfig(InviteUserStepProvider.CONFIG_CLIENT_ID, "invite-client")
.withConfig(InviteUserStepProvider.CONFIG_REDIRECT_URI, "https://app.example.com/welcome")
.build()
).build()).close();
services/src/main/java/org/keycloak/models/workflow/InviteUserStepProviderFactory.java:59
- The runtime step reads these values via
stepModel.getConfig().getFirst(...), but validation usesmodel.get(...). To avoid mismatched semantics (especially if the underlying config supports multi-valued entries), use the same access pattern in validation (e.g.,model.getConfig().getFirst(...)) for consistency and to prevent subtle config parsing differences.
public void validateConfiguration(KeycloakSession session, RealmModel realm, ComponentModel model)
throws ComponentValidationException {
String clientId = model.get(InviteUserStepProvider.CONFIG_CLIENT_ID);
String redirectUri = model.get(InviteUserStepProvider.CONFIG_REDIRECT_URI);
services/src/main/java/org/keycloak/models/workflow/InviteUserStepProviderFactory.java:71
- When
client-idis not configured, the step defaults to the realm system client (viaActionTokenEmail.resolveParams(...)). However, validation currently does not verify that the system client exists and is enabled, which can lead to workflows that validate successfully but always skip sending invites at runtime (CLIENT_UNAVAILABLE). Consider validating the default client path too (e.g., resolve/check system client presence/enabled) and fail fast with aComponentValidationExceptionexplaining how to fix it (configureclient-idor enable the system client).
if (redirectUri != null && clientId == null) {
throw new ComponentValidationException("'redirect-uri' requires 'client-id' to be set");
}
if (clientId != null) {
ClientModel client = realm.getClientByClientId(clientId);
if (client == null) {
throw new ComponentValidationException("Client '" + clientId + "' does not exist");
}
if (redirectUri != null && RedirectUtils.verifyRedirectUri(session, redirectUri, client) == null) {
throw new ComponentValidationException("'redirect-uri' is not a valid redirect URI for client '" + clientId + "'");
}
}
services/src/main/java/org/keycloak/models/workflow/InviteUserStepProviderFactory.java:76
- This validation error does not include which action(s) were invalid, which makes it harder to diagnose misconfiguration. Consider including the configured value(s) in the exception message (e.g., echo
actions) so admins can immediately see what needs correcting.
List<String> actions = model.getConfig().get(InviteUserStepProvider.CONFIG_ACTIONS);
if (actions != null && !RequiredActionsValidator.validRequiredActions(session, actions)) {
throw new ComponentValidationException("Invalid required action configured for 'actions'");
}
docs/documentation/server_admin/topics/workflows/defining-steps.adoc:101
- This is a single very long line; many docs toolchains expect soft-wrapping for readability and cleaner diffs. Consider wrapping this paragraph across multiple lines (and potentially splitting into two paragraphs) while keeping the AsciiDoc meaning unchanged.
The `invite-user` step builds invitation links from the configured hostname, resolved from the realm `frontendUrl` attribute or the `--hostname` option when it is set to a full URL. Because the step runs on a background thread without an active request, one of these must be configured: a workflow containing an `invite-user` step is rejected at creation time with a `400 Bad Request` if none is set. The step is only skipped at runtime if the hostname configuration is removed after the workflow was created.
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostPassiveTestKeycloak CI - Adapter IT Strict Cookies org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#unauthorizedSSOTestKeycloak CI - Adapter IT Strict Cookies org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testMultipleTabsParallelLoginAfterAuthSessionExpirationKeycloak CI - Adapter IT Strict Cookies |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostPassiveTestKeycloak CI - Adapter IT Strict Cookies org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#unauthorizedSSOTestKeycloak CI - Adapter IT Strict Cookies org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testMultipleTabsParallelLoginAfterAuthSessionExpirationKeycloak CI - Adapter IT Strict Cookies |
Closes #49297
Adds a new
invite-userworkflow step that sends an invitation email to the user the workflow is processing, with a one-time action token link.Usage
Drafted with an AI coding agent; reviewed and tested locally.