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

Create initial account when there are no accounts in the system #1708

Merged
merged 1 commit into from
Jun 15, 2024

Conversation

barreiro
Copy link
Collaborator

@barreiro barreiro commented May 16, 2024

This PR adds a mechanism that generates an account that can be used to bootstrap Horreum. That account should be removed once there are other accounts in the system.

Its another step towards removing dependency on keycloak and also custom dev-services.

fixes #1719

@barreiro barreiro requested a review from johnaohara May 16, 2024 20:29
public class SecurityMigration {

private static final Logger LOGGER = Logger.getLogger("SecurityMigration");
@ApplicationScoped public class SecurityInit {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed SecurityMigration to SecuritiInit as the initial account logic needs to occur after any migration (if it occurs).

if (userCount == 0) {
UserService.NewUser user = new UserService.NewUser();
user.user = new UserService.UserData("", INITIAL_ACCOUNT, "Temporary", "Admin", "temp@horreum.com");
user.password = new SecureRandom().ints(RANDOM_PASSWORD_LENGTH, '0', 'z' + 1).collect(StringBuilder::new, StringBuilder::appendCodePoint, StringBuilder::append).toString();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generates a password from random characters from ASCII 0 to z (includes digits, upper and lowercase letters and punctuation --- all characters in the range are printable)


} else if (userCount > 1 && !backend.info(List.of(INITIAL_ACCOUNT)).isEmpty()) {
Log.warnv("Please remove temporary admin account: {0}", INITIAL_ACCOUNT);
// TODO: remove account or add a way to remove accounts
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the moment there is no way to remove an account. would need to implement that feature.

@stalep
Copy link
Member

stalep commented May 20, 2024

@barreiro again, which issue is this solving?

@barreiro barreiro marked this pull request as ready for review May 21, 2024 14:02
@barreiro
Copy link
Collaborator Author

fixes #1719

@barreiro barreiro force-pushed the security-jpa branch 2 times, most recently from 5b800c0 to ec442d7 Compare May 23, 2024 12:55
@johnaohara
Copy link
Member

@barreiro please can you take a look at the test failures?

@barreiro barreiro force-pushed the security-jpa branch 3 times, most recently from 5eceaf4 to d7405cc Compare June 5, 2024 15:43
@stalep
Copy link
Member

stalep commented Jun 6, 2024

@barreiro does these changes require any new/changes documentation?

@barreiro
Copy link
Collaborator Author

barreiro commented Jun 6, 2024

@stalep yes, but I'm planning to do it in a separate PR that addresses all the changes in authentication.

@johnaohara
Copy link
Member

@barreiro if you are planning on adding documentation to cover authentication, please can you add an issue so that we do not forget and can prioritise bringing the docs in line with functionality. Ideally everything (docs, test, changes etc) should be in on PR

@barreiro
Copy link
Collaborator Author

@johnaohara created #1775 to follow up with documentation.

Copy link
Member

@johnaohara johnaohara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, In dev mode, there is a dev-team created by default. This PR provides the initial admin user in dev mode and prod mode, but removes the default team.

We will need;

  • docs to walk users through configuring Horreum before it can be used, e.g. creating a team, users and assigning them to the new team

ideally, we would also provide a wizard to walk new admins through through the process of creating new teams at first startup

user.password = providedBootstrapPassword.orElseGet(() -> getLaunchMode().isDevOrTest() ? "secret" : generateRandomPassword(RANDOM_PASSWORD_DEFAULT_LENGTH));

backend.get().createUser(user);
Log.infov("Created temporary account {0} with password {1}", BOOTSTRAP_ACCOUNT, user.password);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be more prominent in the log

Suggested change
Log.infov("Created temporary account {0} with password {1}", BOOTSTRAP_ACCOUNT, user.password);
Log.infov("\n\n*********************************\nCreated temporary account `{0}` with password `{1}`\n*********************************", BOOTSTRAP_ACCOUNT, user.password);

creates the following output:

2024-06-13 16:05:41,982 INFO  [io.sma.rea.mes.amqp] (vert.x-eventloop-thread-1) SRMSG16203: AMQP Receiver listening address dataset-event
2024-06-13 16:05:42,306 INFO  [io.hyp.too.hor.ser.SecurityBootstrap] (Quarkus Main Thread) 

*********************************
Created temporary account `horreum.bootstrap` with password `secret`
*********************************

2024-06-13 16:05:42,389 INFO  [io.hyp.too.hor.svc.use.KeycloakUserBackend] (Quarkus Main Thread) Added administrator role to user horreum.bootstrap

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. I changed to something similar (3 lines in the output)

Log.info("Migration from keycloak complete");
}

if (getLaunchMode() != LaunchMode.TEST) { // test client woul need the 'realm-admin' role {@link io.hyperfoil.tools.horreum.svc.KeycloakUserServiceTests}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the check for Test mode? the default username/password is going to be the same for dev and test, so can't we just rely on that fact for tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to make this work, by adding HorreumKeycloakTestResourceLyfecycleManager to wrap KeycloakTestResourceLyfecycleManager to setup the OIDC client the right way.

Most tests run with the HorreumTestProfile that uses WireMock to provide authentication, and that obviously does not allow to create/remove users. Luckly (or not 😉) Horreum can work with with an external OIDC provider 😄 so I changed the testsuite to run that way (horreum.roles.provider=database and horreums.roles.database.override=false) to create the bootstrap account in the DB.

The upside of this work is the consistency between all profiles.

private static final String MIGRATION_PROVIDER = "database";
private static final String BOOTSTRAP_ACCOUNT = "horreum.bootstrap";

private static final char[] RANDOM_PASSWRORD_CHARS = ("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789!@#$%^*()-_=+[{]}|;:,<.>").toCharArray();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some special chars that cause passwords to fail when authenticating via curl. There was an instance today where a password containing special chars works via the UI, but does not work with CURL.
I have not had chance to investigate which chars caused auth via curl to fail, but it this will create a random password that is stored in keycloak, i suggest we restrict the special chars from password

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already reduced the set of symbols, but let's stick with alphanumeric

@@ -149,7 +149,7 @@ private static UserRepresentation convertUserRepresentation(UserService.NewUser

CredentialRepresentation credentials = new CredentialRepresentation();
credentials.setType(CredentialRepresentation.PASSWORD);
credentials.setTemporary(true);
// credentials.setTemporary(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is no longer required, please remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed. it was required, but I was now able to work around it,

@barreiro barreiro force-pushed the security-jpa branch 2 times, most recently from 88aa541 to 9aad069 Compare June 13, 2024 20:50
@barreiro
Copy link
Collaborator Author

@johnaohara I have addressed your comments but the one about the default team. I believe creating such a team is part of initial setup, but is not a requirement.

I'll take that into account in #1775

@johnaohara
Copy link
Member

@johnaohara I have addressed your comments but the one about the default team. I believe creating such a team is part of initial setup, but is not a requirement.

I'll take that into account in #1775

Dev mode is about developer productivity. If developers have to go through manual steps to recreate a team and assign a user every time they start horreum in dev mode, that is a regression wrt productivity.

right now, i can start dev mode and immediately create a job and start pushing data into Horreum. I would even argue that we should have a default schema and test so that I do not have to complete those steps.

We can certainly have a conversation about removing the default team, but I think we need to have a conversation about what we think the behaviour should be and make changes that improves user/dev experience rather than tagging this in a PR that creates a default initial user.

Options could be;

1 - a wizard that pops up when Horreum does not contain any teams and asks the user if they want to load dummy data or walks them through creating a new team
2 - a dev mode option to persist the database between restarts, allowing developers to only create the team once on the first startup, but re-use the same database files, containing the same configuration

But lets open an issue and discuss, rather than removing it here and debating in a PR that is linked to an issue that does not refer to removing the default team in dev mode

@barreiro
Copy link
Collaborator Author

ok, fair enough. I don't have a very strong opinion about it. I do agree with your long term plans, specially number two. agree that we need an issue to track it.

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

Successfully merging this pull request may close these issues.

Initial account
3 participants