Repository navigation
Conversation
84c742a to
72fda18
Compare
f7a10aa to
1796d79
Compare
|
@shawkins Comments addressed and this PR is ready for the next round. Highlight of the changes:
|
There was a problem hiding this comment.
This seems like it needs more refinement. The screen shown to the user says temporary in several places - do you plan on updating the template as well?
Also behavior doesn't match the behavior of the other properties. If I set the admin username env variable, that does not become the default in the form.
There was a problem hiding this comment.
Also behavior doesn't match the behavior of the other properties. If I set the admin username env variable, that does not become the default in the form.
At the moment, this works on two levels - it can be set via an Environment Property (KC_BOOTSTRAP_ADMIN_IS_TEMPORARY) or in the theme.properties file. The latter gives a chance of override this in a custom theme. At the moment, the theme override takes the priority over the environment variable to give custom theme authors more flexibility.
I'm open for a discussion if you'd like me to change the mechanism how this works.
There was a problem hiding this comment.
I adjusted the theme for these changes.
There was a problem hiding this comment.
I see that it works on two levels, but I'm not sure that either is needed - in particular the usage of the environment variable since it's not consistent behavior.
The creation of the initial admin via the localhost welcome page also seems like it won't come into play with for most production usage.
Why not just have a checkbox in the form for the initial admin to choose? Are you thinking that that anyone who may has themed this page will want to control the one time operation without leaving it up to the initial admin and/or won't want to update their template to add a checkbox?
cc @vmuzikar
There was a problem hiding this comment.
From the Defense Unicorns UDS perspective, we would mostly need the Welcome Page approach. The platform is wired in such a way, the initial Platform Administrators use Port Forward (via zarf connect command) and create the initial Keycloak Administrator account this way. We treat this account as a final (and not temporary) one.
Looking from this perspective, having a checkbox whether the Administrator account is temporary or not may appear weird to our users. Therefore, we'd still need to apply a custom theme and pretend this checkbox is always "checked" for us. Not the best option for us but we can deal with it.
Since Keycloak has many entry points to create the initial Admin Account (command line, environment variables, initial Realm import and Welcome Page), I tried to deliver a solution for each of them. This way the code is "symmetric". I'm slightly concerns that otherwise this feature would look like "half-baked". But I'll leave the final call to both of you @vmuzikar and @shawkins - how would you like me to approach this?
There was a problem hiding this comment.
in particular the usage of the environment variable since it's not consistent behavior
Exactly.
Additionally, I do not believe a theme prop is something that should affect this, it's not its responsibility, IMHO.
I'm ok with having the ability to set the temporary flag via the Welcome Screen, though. However, we should use a systematic approach: fully leverage the bootstrap-admin-temporary option, i.e. it would accept the env var, the CLI option, etc.
1796d79 to
bd19a46
Compare
|
@shawkins The comments have been addressed and the change is ready for the next round. The major changes are in Welcome Page theme. It now doesn't print information about temporary user if the I'm still considering writing a unit test for it but this PR is also fine without it (this is a bit niche feature). |
53c2973 to
64143b8
Compare
|
Comments addressed and this PR is ready for the next round. Here's the list of changes:
|
There was a problem hiding this comment.
in particular the usage of the environment variable since it's not consistent behavior
Exactly.
Additionally, I do not believe a theme prop is something that should affect this, it's not its responsibility, IMHO.
I'm ok with having the ability to set the temporary flag via the Welcome Screen, though. However, we should use a systematic approach: fully leverage the bootstrap-admin-temporary option, i.e. it would accept the env var, the CLI option, etc.
7267163 to
c34582e
Compare
c34582e to
18c8fbc
Compare
ffbf7fa to
1345388
Compare
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.cluster.SessionFailoverClusterTest#sessionFailover |
For the UDS Platform, having this functionality in WelcomeResource is vital. Putting this into Config seems a bit artificial as this is not really an SPI and there's only 1 implementation that probably won't be overridden. Maybe I'll just add a link to #39123 in the comment so that we can retrofit it later once we have better options. |
Signed-off-by: Sebastian Łaskawiec <sebastian.laskawiec@defenseunicorns.com>
bdba7ea to
4fff581
Compare
It might feel artificial but it's currently the way of doing that. We don't want to create a direct dependency of core KC codebase on Quarkus dist config, hence the abstraction in form of SPI config layer. But it's not a real blocker for me. |
|
|
||
| Both `bootstrap-admin user` and `bootstrap-admin service` commands support the `--is-temporary` parameter. This parameter allows you to treat the created admin account as a permanent one, meaning it will not be marked as temporary and will not be subject to the temporary account restrictions. | ||
|
|
||
| A temporary admin account can also be created with the Welcome Screen that is displayed at the first start of {project_name}. In this case, the temporary account behavior is controlled by the `--bootstrap-admin-temporary` parameter. If the variable or setting is not defined, the account will be created as a temporary one. |
There was a problem hiding this comment.
The --bootstrap-admin-temporary also work with the --bootstrap-admin-* options used for creating admin user/svc account at startup, no? Not just with the Welcome Screen.
There was a problem hiding this comment.
@vmuzikar that wouldn't be consistent with the bootstrap admin commands existing handling. The commands do not consult the bootstrap-admin=* options. Someone could be setting bootstrap-admin-* options in keycloak.conf for example, and we don't want that influencing the bootstrap admin command unintentionally.
So --bootstrap-admin-temporary should just affect the initial admin creation / Welcome Screen.
There was a problem hiding this comment.
So --bootstrap-admin-temporary should just affect the initial admin creation / Welcome Screen.
Yes, that's what I meant. Sorry, I was probably not clear before.
I meant that --bootstrap-admin-temporary should also affect when you create an admin user/service account at startup, not just the Welcome Screen. And that this behaviour should be documented.
|
I disagree with this approach; we don't have a mechanism to auto-delete the admin account at this point, so this option is complete unnecessary. If/when we do some form of auto removal of the admin account it should include a consideration of the scenarios where someone doesn't want to delete the account, but rather configure it properly. I propose a completely different approach to the temporary admin account (never been a big fan of this in the first place). See #41382 |
There was a problem hiding this comment.
I'm not in favour of adding this option, see my comment
|
Closing this one in favor of working on #41388 |
This Pull Request introduces a flag to make the Bootstrap Admin temporary or permanent.
Closes #40305