feat: allow defining initial admin user as env variable #4927

jonasws · 2023-10-04T14:39:02Z

About the changes

This PR adds a way of defining the username and password for the "default admin user" via environment variables. It defaults to using admin and unleash4all, so it should be completely backwards compatible.

Closes #4560

vercel · 2023-10-04T14:39:07Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 5, 2023 2:06pm

vercel · 2023-10-04T14:39:09Z

@jonasws is attempting to deploy a commit to the unleash-team Team on Vercel.

A member of the Team first needs to authorize it.

ivarconr · 2023-10-04T14:51:32Z

src/lib/services/user-service.ts

@@ -128,12 +128,15 @@ class UserService {
        if (userCount === 0) {
            // create default admin user
            try {
-                const pwd = 'unleash4all';
+                const username =
+                    process.env.UNLEASH_DEFAULT_ADMIN_USERNAME ?? 'admin';


I would like to move this to the config object used for all config resolutions.

It should probably be seperate fields on the authentication options, with current values as default:
https://github.com/Unleash/unleash/blob/main/src/lib/types/option.ts#L56

Thanks for the pointer. I'll look into moving these fields there instead.

Does the update of how this is defined/read in 1012c34 make more sense?

ivarconr · 2023-10-04T17:30:52Z

src/lib/types/option.ts

@@ -57,7 +57,10 @@ export interface IAuthOption {
    enableApiToken: boolean;
    type: IAuthType;
    customAuthHandler?: Function;
-    createAdminUser: boolean;


Unfortunately, removing this option would be considered a breaking change. We would need to keep the option, and can then use it to set the default values for the initial user. If you want to configure the user yourself you can do that via the new initalAdminUser option.

Ah, I did not realize at first. I wanted to simplify what would happen, so that one can't define a pair of options that doesn't "make sense".

ivarconr · 2023-10-04T17:34:07Z

src/lib/types/option.ts

@@ -57,7 +57,10 @@ export interface IAuthOption {
    enableApiToken: boolean;
    type: IAuthType;
    customAuthHandler?: Function;
-    createAdminUser: boolean;
+    initialAdminUser: {


This new option makes sense to me. If you set both this and createAdminUser at the same time the values in this option should dictate the result.

The only edge case I can think of is if you set createAdminUser=false and defines values for initialAdminUser. I this case my suggestion would be that we will not try to create the admin user.

What do you think?

As stated in the other comment I'm guessing createAdminUser === false should take precedence, but ideally one shouldn't define both. Perhaps adding a runtime check to warn/throw if createAdminUser: false and initialAdminUser: 'non-null' are both explicitly passed? Also, I think if the env variables for username and passwords are present, this should force createAdminUser to be true. If people pass both a custom object AND use the env variables, I think we'd be wise to consider throwing an error, unless we can reliably resolve these and make the precedence clear in the docs.

chriswk

Thank you for adding this. Unfortunately, as Ivar said, we can't remove the createAdminUser field without it being a breaking change.

Would you be willing to update the PR to keep it?

I think the behaviour you discussed seems reasonable, at least throwing a warn if createAdminUser is false and you've sent in an initialAdminUser.

createAdminUser is true, no initialAdminUser - same behaviour as we had before your patch (admin/unleash4all)
createAdminUser is true and you've sent in an initialAdminUser we should use the initialAdminUser to create the adminUser,
createAdminUser is unset, but an initialAdminUser is passed in, we should create the initialAdminUser.
createAdminUser is unset and no initialAdminUser - do nothing

jonasws · 2023-10-05T07:48:43Z

Thank you for adding this. Unfortunately, as Ivar said, we can't remove the createAdminUser field without it being a breaking change.

Would you be willing to update the PR to keep it?

I think the behaviour you discussed seems reasonable, at least throwing a warn if createAdminUser is false and you've sent in an initialAdminUser.
* createAdminUser is true, no initialAdminUser - same behaviour as we had before your patch (admin/unleash4all)

* createAdminUser is true and you've sent in an initialAdminUser we should use the initialAdminUser to create the adminUser,

* createAdminUser is unset, but an initialAdminUser is passed in, we should create the initialAdminUser.

* createAdminUser is unset and no initialAdminUser - do nothing

Thanks for mapping out the different "logical branches" here. I'm already working on this, so I'll try to update this PR later today.

jonasws · 2023-10-05T08:41:50Z

@chriswk I've updated the behavior in f0d9dc6 to match what we discussed. I feel that this warrants some unit tests, as there are now four "logical branches" to account for. I'll get around to that right away, but feel free to comment on whether you feel the logic matches what you described.

chriswk · 2023-10-05T09:24:31Z

@jonasws - Thank you for the update. Yes, this matches my expectation. Great that you're adding unit tests. Once those are in place and running green I'll give you a 👍 on this.

jonasws · 2023-10-05T10:27:01Z

@jonasws - Thank you for the update. Yes, this matches my expectation. Great that you're adding unit tests. Once those are in place and running green I'll give you a 👍 on this.

Great. Now to the "bad" news: I've run into some real hickups as to how one would actually test the fact that initAdminUser is scheduled via process.nextTick. I'm sure there is a good reason for it being implemented this way, but does anyone have any pointers as to how one would test that in Jest 29? Using fake timers and advancing/running ticks doesn't seem to play well with the fact that we're scheduling a promise. The current tests don't actually test this, as they proceed to explicitly call initAdminUser after constructing the UserService object.

My hunch is that someone faced the same challenge in the past, and decided it wasn't worth the effort. Do we think that still applies here?

chriswk · 2023-10-05T11:21:00Z

Hmm. my gut reaction is to move the logic for what to do into the initAdminUser function. So making initAdminUser something we can call with the config settings. That way you can test the logic without worrying about the process.nextTick(). We're not interested in testing process.nextTick scheduling. We have to trust the engine for that.

jonasws · 2023-10-05T11:26:49Z

Hmm. my gut reaction is to move the logic for what to do into the initAdminUser function. So making initAdminUser something we can call with the config settings. That way you can test the logic without worrying about the process.nextTick(). We're not interested in testing process.nextTick scheduling. We have to trust the engine for that.

That has crossed my mind as well, and also aligns better with how the current test suite is implemented. Thanks for the input.

UPDATE: Refactored the handling and updated/added new unit test cases accordingly in c75a3c4. Seems like it works now, but the logs are a bit flooded during the test run. Should we check "twice" whether we create the user? Both before calling process.nextTick and inside the callback?

jonasws · 2023-10-05T13:05:26Z

Looks like I was able to finally address the snapshot update. Not sure about the other CI checks, but I feel like we're closing in here.

chriswk

Nice. You just saved us some work this quarter.

chriswk · 2023-10-06T07:06:37Z

The other CI checks are on us. I've updated my review. Will squash and merge

Thank you!

jonasws requested review from thomasheartman and dgorton as code owners October 4, 2023 14:39

github-actions bot assigned jonasws Oct 4, 2023

vercel bot deployed to Preview – unleash-monorepo-frontend October 4, 2023 14:41 View deployment

ivarconr reviewed Oct 4, 2023

View reviewed changes

jonasws force-pushed the initial-admin-user-env-variable-override branch from dab926c to 9f07d56 Compare October 4, 2023 16:28

vercel bot deployed to Preview – unleash-monorepo-frontend October 4, 2023 16:29 View deployment

jonasws force-pushed the initial-admin-user-env-variable-override branch from 9f07d56 to 82c1451 Compare October 4, 2023 16:33

vercel bot deployed to Preview – unleash-monorepo-frontend October 4, 2023 16:35 View deployment

ivarconr reviewed Oct 4, 2023

View reviewed changes

ivarconr requested a review from chriswk October 4, 2023 20:08

chriswk requested changes Oct 5, 2023

View reviewed changes

jonasws force-pushed the initial-admin-user-env-variable-override branch from 82c1451 to 7f41ad3 Compare October 5, 2023 08:40

vercel bot deployed to Preview – unleash-monorepo-frontend October 5, 2023 08:44 View deployment

jonasws force-pushed the initial-admin-user-env-variable-override branch from 7f41ad3 to ccb75c2 Compare October 5, 2023 10:51

vercel bot deployed to Preview – unleash-monorepo-frontend October 5, 2023 10:53 View deployment

jonasws force-pushed the initial-admin-user-env-variable-override branch from ccb75c2 to 59b999f Compare October 5, 2023 11:58

vercel bot deployed to Preview – unleash-monorepo-frontend October 5, 2023 12:00 View deployment

jonasws force-pushed the initial-admin-user-env-variable-override branch from 59b999f to 4be64d6 Compare October 5, 2023 12:07

vercel bot deployed to Preview – unleash-monorepo-frontend October 5, 2023 12:09 View deployment

jonasws force-pushed the initial-admin-user-env-variable-override branch from 4be64d6 to aeb41e3 Compare October 5, 2023 12:12

vercel bot deployed to Preview – unleash-monorepo-frontend October 5, 2023 12:15 View deployment

jonasws force-pushed the initial-admin-user-env-variable-override branch from aeb41e3 to 97f2882 Compare October 5, 2023 12:16

vercel bot deployed to Preview – unleash-monorepo-frontend October 5, 2023 12:19 View deployment

jonasws added 2 commits October 5, 2023 14:45

feat: allow defining initial admin user as env variable

2e5a67f

chore: add docs on how to override default user

25d6ac2

jonasws force-pushed the initial-admin-user-env-variable-override branch from 97f2882 to 25d6ac2 Compare October 5, 2023 12:45

vercel bot deployed to Preview – unleash-monorepo-frontend October 5, 2023 14:06 View deployment

chriswk approved these changes Oct 6, 2023

View reviewed changes

chriswk merged commit 80c4a82 into Unleash:main Oct 6, 2023
8 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow defining initial admin user as env variable #4927

feat: allow defining initial admin user as env variable #4927

jonasws commented Oct 4, 2023

vercel bot commented Oct 4, 2023 •

edited

vercel bot commented Oct 4, 2023

ivarconr Oct 4, 2023

ivarconr Oct 4, 2023

jonasws Oct 4, 2023

jonasws Oct 4, 2023 •

edited

ivarconr Oct 4, 2023

jonasws Oct 4, 2023

ivarconr Oct 4, 2023

jonasws Oct 4, 2023

chriswk left a comment •

edited

jonasws commented Oct 5, 2023

jonasws commented Oct 5, 2023

chriswk commented Oct 5, 2023

jonasws commented Oct 5, 2023

chriswk commented Oct 5, 2023

jonasws commented Oct 5, 2023 •

edited

jonasws commented Oct 5, 2023

chriswk left a comment

chriswk commented Oct 6, 2023

feat: allow defining initial admin user as env variable #4927

feat: allow defining initial admin user as env variable #4927

Conversation

jonasws commented Oct 4, 2023

About the changes

vercel bot commented Oct 4, 2023 • edited

vercel bot commented Oct 4, 2023

ivarconr Oct 4, 2023

Choose a reason for hiding this comment

ivarconr Oct 4, 2023

Choose a reason for hiding this comment

jonasws Oct 4, 2023

Choose a reason for hiding this comment

jonasws Oct 4, 2023 • edited

Choose a reason for hiding this comment

ivarconr Oct 4, 2023

Choose a reason for hiding this comment

jonasws Oct 4, 2023

Choose a reason for hiding this comment

ivarconr Oct 4, 2023

Choose a reason for hiding this comment

jonasws Oct 4, 2023

Choose a reason for hiding this comment

chriswk left a comment • edited

Choose a reason for hiding this comment

jonasws commented Oct 5, 2023

jonasws commented Oct 5, 2023

chriswk commented Oct 5, 2023

jonasws commented Oct 5, 2023

chriswk commented Oct 5, 2023

jonasws commented Oct 5, 2023 • edited

jonasws commented Oct 5, 2023

chriswk left a comment

Choose a reason for hiding this comment

chriswk commented Oct 6, 2023

vercel bot commented Oct 4, 2023 •

edited

jonasws Oct 4, 2023 •

edited

chriswk left a comment •

edited

jonasws commented Oct 5, 2023 •

edited