-
-
Notifications
You must be signed in to change notification settings - Fork 740
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
feat: Add configurable default email for initial admin user #6447
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ exports[`should create default config 1`] = ` | |
"initialAdminUser": { | ||
"password": "unleash4all", | ||
"username": "admin", | ||
"email": "[email protected]", | ||
}, | ||
"type": "open-source", | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -267,6 +267,7 @@ const defaultAuthentication: IAuthOption = { | |
initialAdminUser: { | ||
username: process.env.UNLEASH_DEFAULT_ADMIN_USERNAME ?? 'admin', | ||
password: process.env.UNLEASH_DEFAULT_ADMIN_PASSWORD ?? 'unleash4all', | ||
email: process.env.UNLEASH_DEFAULT_ADMIN_EMAIL ?? '[email protected]', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
}, | ||
initApiTokens: [], | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,6 +145,7 @@ test('Should create default user - with provided username and password', async ( | |
initialAdminUser: { | ||
username: 'admin', | ||
password: 'unleash4all!', | ||
email: '[email protected]', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should not be set. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming we are still keeping the email functionality but making it opt-in, it would be beneficial to have a test for it. Would it be better to create a separate test case for the email rather than having it in this one? |
||
}, | ||
}); | ||
|
||
|
@@ -190,6 +191,7 @@ test('Should not create default user - with `createAdminUser` === false', async | |
initialAdminUser: { | ||
username: 'admin', | ||
password: 'unleash4all!', | ||
email: '[email protected]', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should not be set. |
||
}, | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -136,28 +136,32 @@ class UserService { | |||||
): Promise<void> { | ||||||
let username: string; | ||||||
let password: string; | ||||||
let email: string; | ||||||
|
||||||
if ( | ||||||
initialAdminUserConfig.createAdminUser !== false && | ||||||
initialAdminUserConfig.initialAdminUser | ||||||
) { | ||||||
username = initialAdminUserConfig.initialAdminUser.username; | ||||||
password = initialAdminUserConfig.initialAdminUser.password; | ||||||
email = initialAdminUserConfig.initialAdminUser.email; | ||||||
} else { | ||||||
username = 'admin'; | ||||||
password = 'unleash4all'; | ||||||
email = '[email protected]'; | ||||||
} | ||||||
|
||||||
const userCount = await this.store.count(); | ||||||
|
||||||
if (userCount === 0 && username && password) { | ||||||
if (userCount === 0 && username && password && email) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ Getting worse: Complex Conditional Why does this problem occur?A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell. Read more. To ignore this warning click here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we actually need to check for the existence of the email here? The only time this'll be falsy is if you provide an empty string as an env variable, right, but still ask unleash to create the initial admin user? I don't think we should not create the admin user if you provide an empty email 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 good call imo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. email should be optional and only used if it exists. |
||||||
// create default admin user | ||||||
try { | ||||||
this.logger.info( | ||||||
`Creating default user '${username}' with password '${password}'`, | ||||||
`Creating default user '${username}', password '${password}', and email '${email}'`, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can leave the old "with" in here, personally. Not major, but it keeps the format slightly more consistent. Not that it matters much 🤷🏼
Suggested change
|
||||||
); | ||||||
const user = await this.store.insert({ | ||||||
username, | ||||||
email, | ||||||
}); | ||||||
const passwordHash = await bcrypt.hash(password, saltRounds); | ||||||
await this.store.setPasswordHash(user.id, passwordHash); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,7 @@ export interface IAuthOption { | |
initialAdminUser?: { | ||
username: string; | ||
password: string; | ||
email: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this needs to be optional. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the username and password be optional too, in the case that a user only wants to specify email, but not username or password (since the defaults will be used if they are not present)? |
||
}; | ||
initApiTokens: ILegacyApiTokenCreate[]; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,6 +95,7 @@ test('should create initial admin user', async () => { | |
initialAdminUser: { | ||
username: 'admin', | ||
password: 'unleash4all', | ||
email: '[email protected]', | ||
}, | ||
}); | ||
await expect(async () => | ||
|
@@ -116,6 +117,7 @@ test('should not init default user if we already have users', async () => { | |
initialAdminUser: { | ||
username: 'admin', | ||
password: 'unleash4all', | ||
email: '[email protected]', | ||
}, | ||
}); | ||
const users = await userService.getAll(); | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -66,7 +66,7 @@ unleash.start(unleashOptions); | |||||
- `demo` - Only requires an email to sign in (was default in v3) | ||||||
- `none` - _Deprecated_ Turn off authentication completely. If no API token is provided towards /`api/client` or `/api/frontend` you will receive configuration for the "default" environment. We generally recommend you use the `demo` type for simple, insecure usage of Unleash. This auth type has many known limitations, particularly related to personalized capabilities such as favorites and [notifications](../../reference/notifications.md). | ||||||
- `customAuthHandler`: function `(app: any, config: IUnleashConfig): void` — custom express middleware handling authentication. Used when type is set to `custom`. Can not be set via environment variables. | ||||||
- `initialAdminUser`: `{ username: string, password: string} | null` — whether to create an admin user with default password - Defaults to using `admin` and `unleash4all` as the username and password. Can not be overridden by setting the `UNLEASH_DEFAULT_ADMIN_USERNAME` and `UNLEASH_DEFAULT_ADMIN_PASSWORD` environment variables. | ||||||
- `initialAdminUser`: `{ username: string, password: string, email: string} | null` — whether to create an admin user with default password - Defaults to using `admin` and `unleash4all` as the username and password, and `[email protected]` as the email. Can not be overridden by setting the `UNLEASH_DEFAULT_ADMIN_USERNAME`, `UNLEASH_DEFAULT_ADMIN_PASSWORD` and `UNLEASH_DEFAULT_ADMIN_EMAIL` environment variables. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I found a mistake in here that's not yours:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't forget changing the domain in the email here as well, if we end up with a default email There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove the default email and specify that the email is only set if it is provided. |
||||||
- `initApiTokens` / `INIT_ADMIN_API_TOKENS`, `INIT_CLIENT_API_TOKENS`, and `INIT_FRONTEND_API_TOKENS`: `ApiTokens[]` — Array of API tokens to create on startup. The tokens will only be created if the database doesn't already contain any API tokens. Example: | ||||||
|
||||||
```ts | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't own unleash.com, so it would probably be better to do something like "[email protected]". Or maybe something like "[email protected]"? Depending on what we want to change it to, that would obviously apply to all the places that use that email in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather keep our official domain out of it. we can use [email protected] instead in our tests