p2-inc / keycloak-magic-link

Magic Link Authentication for Keycloak
https://phasetwo.io
Other
245 stars 50 forks source link

Magic-Code with Force Create User, etc #41

Closed thebestgin closed 1 year ago

thebestgin commented 1 year ago

I love the new Magic-Code function. Thanky a lot.

Is it possible to enable a Force Create User like in Magic-Link?

Magic-Code

xgp commented 1 year ago

Not currently. Happy to accept a PR in this regard. Should be fairly easy to follow how it's done in the magic link authenticator and token handler.

Arnaud-J commented 1 year ago

I would like to use this feature too.

@thebestgin are you working on this yet? I would be keen to help if needed.

thebestgin commented 1 year ago

Hi @Arnaud-J,

So far I have only looked at the relevant code for Magic Links. For the most part, you can take the code 1 to 1. What is holding me back is the Java framework. Setting up a runnable Java environment for testing takes too long for me. I am a passionate .NET developer.

If you take over the feature I would be very grateful. I would be happy to test the finished feature in a Docker container.

xgp commented 1 year ago

@thebestgin @Arnaud-J I've got some time to do this later this week. Prefer to have PRs and external participation if you can get to it, but otherwise, I can do it on Friday.

Arnaud-J commented 1 year ago

I do not think I will have the time in the coming days. I am not in a hurry about this feature, it is for a project due in a few weeks.

@xgp If you have time this Friday, please go ahead, otherwise I will try to spend some time on it when I can. Also, like @thebestgin, I am happy to test the feature if you start a PR.

xgp commented 1 year ago

Looking at the code, I don't think altering the existing EmailOtpAuthenticator is the best approach. It makes a lot of assumptions about having an existing/authenticated user. I would suggest building an Authenticator that creates a new user based on an input username/email. Set that and the EmailOtpAuthenticator as REQUIRED, and you get the same effect. I don't think it needs to be added to this library.

Something like


public class NewUserAuthenticator extends UsernamePasswordForm  {

  @Override
  public void authenticate(AuthenticationFlowContext context) {
    String attemptedUsername = getAttemptedUsername(context);
    if (attemptedUsername == null) {
      super.authenticate(context);
    } else {
      log.infof(
          "Found attempted username %s from previous authenticator, skipping login form",
          attemptedUsername);
      action(context);
    }
  }

  @Override
  public void action(AuthenticationFlowContext context) {
    MultivaluedMap<String, String> formData = context.getHttpRequest().getDecodedFormParameters();

    String email = trimToNull(formData.getFirst(AuthenticationManager.FORM_USERNAME));
    if (email == null) {
      email = getAttemptedUsername(context);
    }
    if (email == null) {
      context.getEvent().error(Errors.USER_NOT_FOUND);
      Response challengeResponse = challenge(context, getDefaultChallengeMessage(context), FIELD_USERNAME);
      context.failureChallenge(AuthenticationFlowError.INVALID_USER, challengeResponse);
      return;
    }

    EventBuilder event = context.newEvent();

    UserModel user = MagicLink.getOrCreate(
        context.getSession(),
        context.getRealm(),
        email,
        true,
        false,
        false,
        MagicLink.registerEvent(event));

    context
        .getAuthenticationSession()
        .setAuthNote(AbstractUsernameFormAuthenticator.ATTEMPTED_USERNAME, email);

    context.setUser(user);

    context.success();
  }

  private String getAttemptedUsername(AuthenticationFlowContext context) {
    if (context.getUser() != null && context.getUser().getEmail() != null) {
      return context.getUser().getEmail();
    }
    String username =
        trimToNull(context.getAuthenticationSession().getAuthNote(ATTEMPTED_USERNAME));
    if (username != null) {
      if (isValidEmail(username)) {
        return username;
      }
      UserModel user = context.getSession().users().getUserByUsername(context.getRealm(), username);
      if (user != null && user.getEmail() != null) {
        return user.getEmail();
      }
    }
    return null;
  }

  @Override
  protected boolean validateForm(
      AuthenticationFlowContext context, MultivaluedMap<String, String> formData) {
    return validateUser(context, formData);
  }

  @Override
  protected Response challenge(
      AuthenticationFlowContext context, MultivaluedMap<String, String> formData) {
    LoginFormsProvider forms = context.form();
    if (!formData.isEmpty()) forms.setFormData(formData);
    return forms.createLoginUsername();
  }

  @Override
  protected Response createLoginForm(LoginFormsProvider form) {
    return form.createLoginUsername();
  }

  @Override
  protected String getDefaultChallengeMessage(AuthenticationFlowContext context) {
    return context.getRealm().isLoginWithEmailAllowed()
        ? Messages.INVALID_USERNAME_OR_EMAIL
        : Messages.INVALID_USERNAME;
  }
}
Arnaud-J commented 1 year ago

I like the new authenticator approach, it allow to decorrelate signin and user creation and let admins build their flows more explicitly.

I also agree that this feature could constitute another extension. But then the same observation can be made about the email otp feature. I think it could sit in its own extension rather than the magic link extension.

Also, if a separate extension is to be built, we would not be able to use MagicLinks methods like MagicLink.getOrCreate(...) which makes sense to me.

@xgp what do you think? Could this extension sit in p2-inc org? If I end up building this, I am willing to share ownership.

xgp commented 1 year ago

we would not be able to use MagicLinks methods like MagicLink.getOrCreate(...)

Of course you would. If you're already loading this jar for the EmailOtpAuthenticator, you can just import the MagicLink class in your code.

share ownership

The next time someone volunteers time out of their day to write nearly complete code to satisfy your request, don't talk about "sharing ownership" to something that isn't yours. Instead, try expressing some gratitude. Just an idea.

Arnaud-J commented 1 year ago

I believe I expressed myself poorly, sorry for that. I will try to elaborate.

My idea was to bootstrap a new GitHub repository (with CI, issue tracking and all that) for the feature you propose : an authenticator whose purpose is to create users on the fly when added as a sub-step in an authentication flow.

Also, I did not read your code thoroughly and I did not realize it was already complete and ready to use. Sorry about that.

By saying "share ownership", I did not mean to offense you nor steal your work. I was thinking that I could spend some time to setup this repository with your code and some setup for building the extension in order to make it available to the world to use. Thus, I was wondering if you were interested to include this repository in the p2-inc organization on GitHub, that's it.

Again, no disrespect intended for you nor your work. I would understand that you don't want to hear about this feature anymore, in which case I will either build it and package it, and open-source it on GitHub or simply throw it in the existing code base and build locally if I lack time.

Hope you understand where I stand and know that I am grateful for your work since I use it in projects.

xgp commented 1 year ago

@Arnaud-J Feel free to create your own extension. I think that this repo isn't the place for it.