Implement "Select Organization" Authenticator.

MGLL commented 4 months ago

Linked to: https://github.com/p2-inc/keycloak-orgs/issues/169

Changes:

Added ActiveOrganizationAuthenticator
Added ActiveOrganizationAuthenticatorFactory
Added select-organization.ftl
Added Documentation
Did Manual Testing
Add built-in browser and direct grant flows
Add Cypress tests
Extend documentation
Rebased on main

Manual Testing report:

Browser Flow

Authenticator is Enabled

prompt=select_account is not used
- User has no Organizations = User is not required to select an organization. OK
- User has 1 or more Organization = User is not required to select an organization. OK
prompt=select_account is used
- User has no Organizations = User get "You are not part of any organization, Contact an Administrator." error. OK
- User has 1 Organization = User doesn't have to select an organization. OK
- User has 2 Organization = User have to select an organization. OK
account_hint=org-id is used (with a right organization)
- User has no Organizations = User get "Invalid Organization." error. OK
- User has 1 or more Organization = User doesn't have to select an organization. OK
account_hint=org-id is used (with a wrong organization)
- All users get "Invalid Organization." error. OK

Authenticator is Disabled

prompt=select_account is used - Users doesn't have to choose an organization. OK
account_hint=org-id is used - Account hint is ignored. OK

Direct Grant Flow

Authenticator is Enabled

account_hint=org-id is used with a right organization = Authentication is successfull. OK
account_hint=org-id is used with a wrong organization = Authentication fails with a 401 status. OK

All case

In all case, if the authentication is successfull, the org.ro.active attribute is updated.
In all case, if the authentication is failure, the org.ro.active attribute isn't updated.

xgp commented 4 months ago

@MGLL I looked over this, and I like the direction. I don't see any problems with the flow implementations. Please let me know when you're ready to take it out of draft, and I'll do a full review. I don't see a problem with leaving the cypress testing for later as long as we mark it as new/experimental.

MGLL commented 4 months ago

Hi @xgp I have setup locally cypress container 2 days ago. I will work on tests, and it should be ready next week.

Also, I will improve the documentation regarding using account_hint for switching org (need to add the select organization step after cookies) and add a default authentication flow like you did for magic link (I need it anyway to simplify test setup).

Best

MGLL commented 4 months ago

Changes:

Add built-in browser and direct grant flows
Add Cypress tests
Extend documentation
Rebased on main

Edited original message.

Note that test got slower as now it runs 2 testcontainers (Keycloak & Cypress, cypress use the keycloak testcontainers to run the tests).

Also note that I tried to explore the "Required actions" approach without success as of now (still discovering a bit the Required actions flow and how I could use evaluateTriggers() or if I should use it. Tried to use it but landed into an infinite loop during login at select org step.).

@xgp open for Review.

There will be surely some improvement regarding Cypress, tried to check Best Practices and do some training.

MGLL commented 3 months ago

@xgp nothing on my side. Should I wait for the v24 release? (so I can apply that after the "big" update, less things to deals with for you).

@pnzrr I have did the last changes, please double check if I missed something (again).

xgp commented 3 months ago

@MGLL Thanks. We're in the middle of validating all our extensions for 24. I won't merge this until we get that done, so "yes" this will be in the 24 release.

xgp commented 3 months ago

24 updates are merged, so this may need to be rebased and conflicts resolved. Otherwise, excited to get this in asap.

MGLL commented 3 months ago

Hello @xgp @pnzrr, I have rebased on main, fixed conflict and update cypress tests according to v24 changes. Also I have added

else if (event instanceof PostMigrationEvent) {
   log.debug("PostMigrationEvent");
   if (KC_ORGS_SKIP_MIGRATION == null) {
      log.info("initializing active organization user profile attribute following migration");
      KeycloakModelUtils.runJobInTransaction(factory, this::postMigrationCreateAuthFlow);
   }
}

to create built-in Authentication Flows on PostMigrationEvent (for existing realms). feel free to check again

Best

xgp commented 3 months ago

I'm still not getting a clean test run. See the log: build.log.txt

Also, it looks like cypress runs something as root (or maybe this is because docker is running as root) and makes any subsequent clean fail. Is there a way to configure testcontainers to run docker as the current user so this won't happen?:

[garth:keycloak-orgs (feat/active-org-authenticator)]$ mvn clean
[INFO] Scanning for projects...
[INFO]
[INFO] -----------------< io.phasetwo.keycloak:keycloak-orgs >-----------------
[INFO] Building keycloak-orgs 0.68-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO]
[INFO] --- maven-clean-plugin:2.5:clean (default-clean) @ keycloak-orgs ---
[INFO] Deleting /home/garth/projects/saascrud/keycloak-orgs/target
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.198 s
[INFO] Finished at: 2024-03-25T23:06:30+01:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-clean-plugin:2.5:clean (default-clean) on project keycloak-orgs: Failed to clean project: Failed to delete /home/garth/projects/saascrud/keycloak-orgs/target/test-classes/e2e/cypress/reports/mochawesome/mochawesome.json -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
[garth:keycloak-orgs (feat/active-org-authenticator)]$ ls -lah /home/garth/projects/saascrud/keycloak-orgs/target/test-classes/e2e/cypress/reports/mochawesome/mochawesome.json
-rw-r--r-- 1 root root 14K Mar 25 23:04 /home/garth/projects/saascrud/keycloak-orgs/target/test-classes/e2e/cypress/reports/mochawesome/mochawesome.json

MGLL commented 3 months ago

I'm still not getting a clean test run. See the log: build.log.txt

Also, it looks like cypress runs something as root (or maybe this is because docker is running as root) and makes any subsequent clean fail. Is there a way to configure testcontainers to run docker as the current user so this won't happen?:

[garth:keycloak-orgs (feat/active-org-authenticator)]$ mvn clean
[INFO] Scanning for projects...
[INFO]
[INFO] -----------------< io.phasetwo.keycloak:keycloak-orgs >-----------------
[INFO] Building keycloak-orgs 0.68-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO]
[INFO] --- maven-clean-plugin:2.5:clean (default-clean) @ keycloak-orgs ---
[INFO] Deleting /home/garth/projects/saascrud/keycloak-orgs/target
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.198 s
[INFO] Finished at: 2024-03-25T23:06:30+01:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-clean-plugin:2.5:clean (default-clean) on project keycloak-orgs: Failed to clean project: Failed to delete /home/garth/projects/saascrud/keycloak-orgs/target/test-classes/e2e/cypress/reports/mochawesome/mochawesome.json -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
[garth:keycloak-orgs (feat/active-org-authenticator)]$ ls -lah /home/garth/projects/saascrud/keycloak-orgs/target/test-classes/e2e/cypress/reports/mochawesome/mochawesome.json
-rw-r--r-- 1 root root 14K Mar 25 23:04 /home/garth/projects/saascrud/keycloak-orgs/target/test-classes/e2e/cypress/reports/mochawesome/mochawesome.json

Hi @xgp, yes, before I used

  private String getUserSID() {
    if (System.getProperty("os.name").startsWith("Windows")) {
      return new com.sun.security.auth.module.NTSystem().getUserSID();
    } else {
      return Long.toString(new com.sun.security.auth.module.UnixSystem().getUid());
    }
  }

with

CypressContainer cypressContainer = new CypressContainer()
            .withCreateContainerCmdModifier(cmd -> cmd.withUser(getUserSID()))

which was working fine on my Linux. However on MacOS (Mac M1), the cypress container doesn't start and give this error:

2024-03-26 08:06:44 npm ERR! code EACCES
2024-03-26 08:06:44 npm ERR! syscall mkdir
2024-03-26 08:06:44 npm ERR! path /.npm
2024-03-26 08:06:44 npm ERR! errno -13
2024-03-26 08:06:44 npm ERR! 
2024-03-26 08:06:44 npm ERR! Your cache folder contains root-owned files, due to a bug in
2024-03-26 08:06:44 npm ERR! previous versions of npm which has since been addressed.
2024-03-26 08:06:44 npm ERR! 
2024-03-26 08:06:44 npm ERR! To permanently fix this problem, please run:
2024-03-26 08:06:44 npm ERR!   sudo chown -R 503:0 "/.npm"
2024-03-26 08:06:44 
2024-03-26 08:06:44 npm ERR! Log files were not written due to an error writing to the directory: /.npm/_logs
2024-03-26 08:06:44 npm ERR! You can rerun the command with `--loglevel=verbose` to see the logs in your termina

I tried multiple things to fix it, the only working fix was to remove the withCreateContainerCmdModifier(cmd -> cmd.withUser(getUserSID())). I'm wondering if we don't need to 'customize' a bit the testcontainer image by adjusting the files permissions to a specific user (like 10001) and run tests with this userSID.

MGLL commented 3 months ago

Managed to make the tests work on my MacOS by using Podman.

In case, I used those env vars for running the test:

DOCKER_HOST=unix://$(podman machine inspect --format '{{.ConnectionInfo.PodmanSocket.Path}}')
TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE=/var/run/docker.sock
TESTCONTAINERS_RYUK_DISABLED=true

I will push the reverted changes.

MGLL commented 3 months ago

Made several changes to improve code quality.

xgp commented 2 months ago

@MGLL The new changes look great. Thanks for cleaning things up. It's very easy to read now.

Regarding the tests, I'm on linux and still haven't been able to get a clean build, even with the environment variables set. If we can't get them to run cleanly on linux (I believe the github action is using ubuntu), what do you think about separating the tests that require cypress and running them only when a profile variable is present? (e.g. https://www.baeldung.com/maven-integration-test#surefire)

MGLL commented 2 months ago

@xgp I have put the cypress tests under a specific profile (-P cypress-tests) as recommended.

But, I think I found the actual problem. I was using AbstractOrganizationTest to extends CypressOrganizationTest which seems to have been the cause of conflicts in the tests. So, I created an AbstractCypressOrganizationTest to run the Cypress tests separately (clean container). Which work fine on my Linux (kubuntu) now with maven clean test -P cypress-tests.

If you still have the files from the tests ran in root config (before my rollback), I recommend removing it and re-run the tests.

xgp commented 2 months ago

@MGLL It looks like the BeforeAll method of AbstractCypressOrganizationTest still gets run even if include.cypress is false which is causing a problem to run it in CI https://github.com/p2-inc/keycloak-orgs/actions/runs/8621298057/job/23629834270#step:5:352

MGLL commented 2 months ago

@xgp I was going to fix it, but I see you already did what I planned to do (move condition to AbstractCypress and add it in the BeforeAll), I missed that, sorry. Let me know if the issue remains, I will take some time tomorrow to see what can be done.

p2-inc / keycloak-orgs