feat: Add configurable default email for initial admin user #6447

00Chaotic · 2024-03-06T11:35:52Z

Ensures initial admin user is created with a default email so that it can be accessed in demo mode. Also allows for custom email to be used, similar to existing username and password config.

Closes #6398

vercel · 2024-03-06T11:35:57Z

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	Mar 6, 2024 11:38am

vercel · 2024-03-06T11:35:58Z

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

A member of the Team first needs to authorize it.

codescene-delta-analysis

❌ Code Health Quality Gates: FAILED

Declining Code Health: 1 findings(s) 🚩
Improving Code Health: 0 findings(s) ✅
Affected Hotspots: 0 files(s) 🔥

Recommended Review Level: Detailed -- Inspect the code that degrades in code health.
View detailed results in CodeScene

🚩 Declining Code Health (highest to lowest):

Complex Conditional user-service.ts: UserService.initAdminUser

codescene-delta-analysis · 2024-03-06T11:36:40Z

src/lib/services/user-service.ts

        }

        const userCount = await this.store.count();

-        if (userCount === 0 && username && password) {
+        if (userCount === 0 && username && password && email) {


❌ Getting worse: Complex Conditional
UserService.initAdminUser increases from 1 complex conditionals with 2 branches to 1 complex conditionals with 3 branches, threshold = 2

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.}

thomasheartman

Hey! Thanks for putting the work in and creating the PR 😄 I think this a good addition.

I've got some minor comments left inline (including stuff like which email address to use etc), but there is one point specifically that I think bears some extra discussion:

Previously, there was no way to log in as the admin user in demo mode, but now you can by default. Will this potentially cause any security concerns where anyone can log in as the admin where they couldn't before?

Would it be more sensible to default to not giving the admin an email, but giving you the option to add an email on startup?

thomasheartman · 2024-03-06T12:20:50Z

src/lib/__snapshots__/create-config.test.ts.snap

@@ -22,6 +22,7 @@ exports[`should create default config 1`] = `
    "initialAdminUser": {
      "password": "unleash4all",
      "username": "admin",
+      "email": "[email protected]",


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.

Suggested change

"email": "admin@unleash.com",

"email": "admin@getunleash.io"

Good catch!

I would rather keep our official domain out of it. we can use [email protected] instead in our tests

thomasheartman · 2024-03-06T12:26:08Z

src/lib/services/user-service.ts

        }

        const userCount = await this.store.count();

-        if (userCount === 0 && username && password) {
+        if (userCount === 0 && username && password && email) {


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 🤔

+1 good call imo

thomasheartman · 2024-03-06T12:27:29Z

website/docs/using-unleash/deploy/configuring-unleash.md

@@ -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.


I think I found a mistake in here that's not yours:

Suggested change

- `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.

- `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 be overridden by setting the `UNLEASH_DEFAULT_ADMIN_USERNAME`, `UNLEASH_DEFAULT_ADMIN_PASSWORD` and `UNLEASH_DEFAULT_ADMIN_EMAIL` environment variables.

Don't forget changing the domain in the email here as well, if we end up with a default email

Please remove the default email and specify that the email is only set if it is provided.

thomasheartman · 2024-03-06T12:28:27Z

src/lib/services/user-service.ts

            // create default admin user
            try {
                this.logger.info(
-                    `Creating default user '${username}' with password '${password}'`,
+                    `Creating default user '${username}', password '${password}', and email '${email}'`,


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

`Creating default user '${username}', password '${password}', and email '${email}'`,

`Creating default user '${username}' with password '${password}' and email '${email}'`,

sighphyre

I don't think I have much input here. This looks good as an overall approach. I think @thomasheartman's poke about the email is a great suggestion though

daveleek · 2024-03-06T13:17:49Z

Would it be more sensible to default to not giving the admin an email, but giving you the option to add an email on startup?

Seeing as this is a feature that's in use I would say this should be a requirement, that it has to be optional and that it has to be opt-in. Otherwise others setups will change as a consequence

thomasheartman · 2024-03-06T13:21:08Z

Seeing as this is a feature that's in use I would say this should be a requirement, that it has to be optional and that it has to be opt-in. Otherwise others setups will change as a consequence

Yeah, that's fair. I'm leaning towards the same. However, seeing as admin users are only created on initial startup (when there's no existing users), we could potentially be more lenient, because it won't change any existing installs. However, I agree that we should err on the side of caution here and make it opt-in instead.

ivarconr · 2024-03-06T13:30:12Z

src/lib/types/option.ts

@@ -69,6 +69,7 @@ export interface IAuthOption {
    initialAdminUser?: {
        username: string;
        password: string;
+        email: string;


this needs to be optional.

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)?

ivarconr · 2024-03-06T13:31:53Z

src/lib/create-config.ts

@@ -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]',


unleash.com is not associated with the Unleash project.

It should be undefined if not set. We should not force it on everyone.

ivarconr · 2024-03-06T13:33:37Z

src/lib/services/user-service.test.ts

@@ -145,6 +145,7 @@ test('Should create default user - with provided username and password', async (
        initialAdminUser: {
            username: 'admin',
            password: 'unleash4all!',
+            email: '[email protected]',


should not be set.

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?

ivarconr · 2024-03-06T13:33:48Z

src/lib/services/user-service.test.ts

@@ -190,6 +191,7 @@ test('Should not create default user - with `createAdminUser` === false', async
        initialAdminUser: {
            username: 'admin',
            password: 'unleash4all!',
+            email: '[email protected]',


should not be set.

ivarconr · 2024-03-06T13:34:32Z

src/lib/services/user-service.ts

        }

        const userCount = await this.store.count();

-        if (userCount === 0 && username && password) {
+        if (userCount === 0 && username && password && email) {


email should be optional and only used if it exists.

ivarconr · 2024-03-06T13:39:18Z

Hi, I suggest that email needs to be optional. This raises a new challenge, what if the user wants to create an initial admin user, but forget to specify the email the first time Unleash runs with the flag?

Should we always do an upsert of the admin user if the flag is specified?

An alternative solution to the problem described in #6398 is to change the input form of demo-login to not require email if the input is simply "admin".

00Chaotic · 2024-03-07T07:30:01Z

Thanks all, it's definitely a good idea to make the admin email an opt-in and remove the default.

An alternative solution to the problem described in #6398 is to change the input form of demo-login to not require email if the input is simply "admin".

@ivarconr Would this not still present the security issue of anyone being able to access the admin user?

I'll be making the admin email optional for now while we continue discussion on the whether to always do an upsert if the flag is specified, or whether to go with an alternate solution entirely.

thomasheartman · 2024-03-13T09:04:38Z

@00Chaotic Sorry for the delay here 🙇🏼

Would this not still present the security issue of anyone being able to access the admin user?

As long as the admin user can log in without a password, that risk is present. So whether we accept admin instead of an email or require the admin user to have an email set, doesn't really make a difference, I think. Do you disagree?

I'll be making the admin email optional for now while we continue discussion on the whether to always do an upsert if the flag is specified, or whether to go with an alternate solution entirely.

Is there a third way where we don't do an upsert, but allow you to set the admin email only when it creates default users? That may not be very useful, though. I can imagine that you often only realize that you want the admin to be able to log in after you've set up Unleash.

But actually, scratch that. I think modifying the demo auth middleware (src/lib/middleware/demo-authentication.ts) instead and adding a special case that logs in the admin user would be a better solution.

I think this would be beneficial because:

It would probably be a smaller change in terms of lines of code.
It would definitely be a smaller change in terms of potential impact (and unforeseen side effects).
It solves the same problem, but in a way where you can easily turn that capability off again if you want to (so it’s reversible, even on the instance level).
It feels like a more direct solution for a problem that only affects a very small percentage of our users.

It should only work if you've started Unleash with a [email protected] flag (or the corresponding [email protected] env var), meaning you can just as easily turn the feature off.

It would also be an option to allow them to log in with just admin, but that would require some UI rework etc (because the input form has type "email"), so requiring the admin login to also be an email would minimize the amount of changes we need. However, it does introduce the possibility that someone has previously created a user with the admin email (or done so by mistake when the feature was turned off). That said, that is definitely edge case territory and might be okay to disregard.

Let me know your thoughts and we can take it from there ☺️

thomasheartman · 2024-03-13T15:27:51Z

Small addendum to the above comment: it appears that we don't do any validation on the server-side that you fill out an email, so it may be possible to just update the UI a bit.

Ivar had a suggestion that is to set the input field's type to be:

type={value === 'admin' ? "text" : "email"}

That might work. Of course, we could also use the pattern attribute and make a regex, but that feels like it might be more error prone.

If we go that way, then admin could log in with admin and the flag could change to something like --authDemoAllowAdminLogin.

thomasheartman · 2024-03-13T15:28:51Z

Of course, there's nothing stopping a general user from using the same username (even today, with a bit of UI inspection), but that's the same as if you were to use an email.

00Chaotic · 2024-03-14T02:52:09Z

Thanks for getting back to me @thomasheartman

As long as the admin user can log in without a password, that risk is present. So whether we accept admin instead of an email or require the admin user to have an email set, doesn't really make a difference, I think. Do you disagree?

Slightly disagree, while it's by no means as secure as a password, requiring the user to specify an email rather than providing a default one would add a level of randomness/variance to improve security. Of course, this is also influenced by how the user specifies the email - if they just hardcode it in then it's less secure than if they treated it like a secret and loaded it in from elsewhere.

But actually, scratch that. I think modifying the demo auth middleware (src/lib/middleware/demo-authentication.ts) instead and adding a special case that logs in the admin user would be a better solution.

Just to confirm, do you mean we add a case to the /auth/demo/login endpoint to just log them in as the default admin user if the input is admin instead of an email?

While I feel that would be less secure than forcing a custom-set email, you're right in that neither of them are super secure, so this solution would be fine. I agree that there is less potential for unforeseen impacts this way.

I can go ahead and make the change if we're all happy with this.

N.B. Do we know if there is any logic for the demo auth mode that may break if a user does not have an email? Any logging or user retrieval functionality that assumes the user has an email when using demo mode?

thomasheartman · 2024-03-14T07:47:24Z

Slightly disagree, while it's by no means as secure as a password, requiring the user to specify an email rather than providing a default one would add a level of randomness/variance to improve security. Of course, this is also influenced by how the user specifies the email - if they just hardcode it in then it's less secure than if they treated it like a secret and loaded it in from elsewhere.

That's fair. But I don't think that people in general will rotate the email if they set it. That seems like a lot of work. I think that if you want security, then you shouldn't be using demo auth.

Just to confirm, do you mean we add a case to the /auth/demo/login endpoint to just log them in as the default admin user if the input is admin instead of an email?

While I feel that would be less secure than forcing a custom-set email, you're right in that neither of them are super secure, so this solution would be fine. I agree that there is less potential for unforeseen impacts this way.

Yeah, that's what I'm saying. Except there is no validation happening on the API level, so all you'd need to update is the middleware and the UI code saying that the input has to be an email.

N.B. Do we know if there is any logic for the demo auth mode that may break if a user does not have an email? Any logging or user retrieval functionality that assumes the user has an email when using demo mode?

You're free to go check it out yourself, but there is nothing as far as I can tell. You can try yourself by logging into the the demo instance by providing a string that isn't an email. You'll have to manually change the input's type (by inspecting it in the browser and removing the type="email" attribute from the input), but logging in (and logging in again later) works just fine.

I can go ahead and make the change if we're all happy with this.

We've talked about it internally, and this seems like a good way to solve it in our opinion. This would mean:

update the demo login input (when the flag is present) to accept admin OR an email (using the solution provided in a previous comment)
Update the middleware to log in the admin user if the username is admin and the flag is present
Update the env var / flag parsing to accept the new flag
Update the docs

There may of course be some other minor changes to make, but that should be the brunt of it.

00Chaotic · 2024-03-14T07:58:46Z

Awesome, I'll get a change pushed out as soon as I can. I'll be making a new branch and PR and linking to this one for context

00Chaotic · 2024-03-14T08:53:29Z

I've realised we can't use the default admin username/password for userService.loginUser() in the middleware since they may have set a custom one. Is there a way to retrieve the credentials used in the creation of the default admin user and/or a more appropriate function I should be using for the login?

thomasheartman · 2024-03-14T09:54:37Z

I've realised we can't use the default admin username/password for userService.loginUser() in the middleware since they may have set a custom one. Is there a way to retrieve the credentials used in the creation of the default admin user and/or a more appropriate function I should be using for the login?

Right! If you follow the login flow in the demo auth, you'll see that it uses loginUserWithoutPassword and then loginUserSSO under the hood, which performs a query for the user and logs them in. If you follow the same logic and instead use this query, it appears to work as expected:

            if (email === 'admin') {
                user = await this.store.getByQuery({ id: 1 });
            }

Now, you could of course update the loginUserSSO method, but I'd be inclined to instead create a new method that duplicates the logic, but handles logging in the admin user with a password specifically instead (to avoid polluting the SSO method and to avoid any potential mixups in the future). Something like loginDemoAuthAdmin.

thomasheartman · 2024-03-14T09:55:42Z

I'd probably also add a check for the flag in that specific method to ensure that it's never invoked successfully without the flag being present.

00Chaotic · 2024-03-14T13:19:06Z

Seems the /api/admin/ endpoint checks whether the user has an email (which the admin user won't have). From a quick look I wasn't able to find where this is used, do we have any context around why that check was added?

thomasheartman · 2024-03-14T13:22:12Z

Seems the /api/admin/ endpoint checks whether the user has an email (which the admin user won't have). From a quick look I wasn't able to find where this is used, do we have any context around why that check was added?

Let me do some digging for you around that. I think we can get away with checking whether the user has an email OR if their username is admin, but I wanna make doubly sure before we go ahead with it.

00Chaotic · 2024-03-15T01:39:44Z

Any idea which tests would be best suited to add these new cases to? Seems there are multiple that hit the /auth/demo/login endpoint but moreso just to be able to perform further testing actions like creating toggles and environments, rather than testing cases for that endpoint itself.

thomasheartman · 2024-03-15T09:08:17Z

Doesn't look like we have any direct tests for that middleware yet 🤔 What tests did you find?

I think I'd look at src/lib/middleware/oss-authentication.test.ts or src/lib/middleware/no-authentication.test.ts and start adding some tests in a new src/lib/middleware/demo-authentication.test.ts file.

We probably want to test that:

When started with the right flag, you can log in as admin with admin
When started without the flag, logging in with admin does not work (or at least doesn't give you the admin user)
If a user already exists with the admin username (not the admin user), logging in with admin still gives you the admin user and not the non-admin user with the same username (remember, if you fiddle with the UI, you can log in with anything today, but Unleash will just assign that as the user's email)
Logging in with a different user (whether with email or an arbitrary username) does not give you the admin user.

00Chaotic · 2024-03-16T11:05:30Z

Thanks, I'm a little unfamiliar with testing in JS and with how the tests are set up in this project specifically. I've seen that /src/test/fixtures/ has various mock implementations of multiple stores, but only one service. I've gotten the tests partially working by setting up the stores and services the same way they are set up in oss-authentication.test.ts, but the tests error when the demo login logic is executed because the relevant root Admin and Editor roles do not exist in the mock stores.

Would you happen to know what the correct way to create the default root roles would be? From what I can see, it seems the root roles are actually created through SQL rather than by using the access service, which only allows for roles of type root-custom and custom to be created.

If it helps, I can push my current code to a branch to give you a better idea of what it looks like.

thomasheartman · 2024-03-18T09:51:07Z

Right. Yeah, setup can be a bit weird, but it should be very possible to achieve. So yeah, please make a branch that I can have a look at and I'll see what I can do 🙋🏼

00Chaotic · 2024-03-19T04:17:49Z

Haha the setup has definitely been tricky. I'm on Windows and I assume this project is mainly aimed at development using Mac/Linux. I ended up using WSL to try running it which still seems to error out on the initial yarn run among other things but I've gotten it to a state where it can run so...small victories.

I've pushed the changes to https://github.com/00Chaotic/unleash/tree/feat/allow_admin_login_using_demo_auth. I believe it's just the tests left that need to be sorted out.

thomasheartman · 2024-03-19T07:14:18Z

Cool; I'll have a look 🙋🏼 As for development, I know most of our internal devs are on Linux or macOS, but we do have some running Windows. I haven't heard about any issues with it, though. @Tymek, you're on Windows, aren't you? Do you need to do some weird magic to make it work?

thomasheartman · 2024-03-19T07:57:18Z

@00Chaotic , I don't have push access to your branch, but this appears to work (the first test fails, but I'll leave it to you to look into that, if you don't mind):

import { IAuthType } from '../server-impl';
import dbInit, { ITestDb } from '../../test/e2e/helpers/database-init';
import { IUnleashStores } from '../types';
import { setupAppWithCustomAuth } from '../../test/e2e/helpers/test-helper';

let db: ITestDb;
let stores: IUnleashStores;

beforeAll(async () => {
    db = await dbInit('demo_auth_serial');
    stores = db.stores;
});

afterAll(async () => {
    await db?.destroy();
});

const getApp = (adminLoginEnabled: boolean) =>
    setupAppWithCustomAuth(stores, () => {}, {
        authentication: {
            authDemoAllowAdminLogin: adminLoginEnabled,
            type: IAuthType.DEMO,
            createAdminUser: true,
        },
    });

test('should allow login with admin user if flag enabled', async () => {
    const app = await getApp(true);
    return app.request
        .post(`/auth/demo/login`)
        .send({ email: 'admin' })
        .expect(200)
        .expect((res) => {
            expect(res.body.id).toBe(1);
            expect(res.body.username).toBe('admin');
        });
});

test('should create regular user with flag enabled', async () => {
    const app = await getApp(true);
    return app.request
        .post(`/auth/demo/login`)
        .send({ email: '[email protected]' })
        .expect(200)
        .expect((res) => {
            expect(res.body.email).toBe('[email protected]');
            expect(res.body.id).not.toBe(1);
        });
});

test('should return 403 for admin user if flag disabled', async () => {
    const app = await getApp(false);
    return app.request
        .post(`/auth/demo/login`)
        .send({ email: 'admin' })
        .expect(403);
});

00Chaotic · 2024-03-20T07:17:58Z

Hmm the tests seem to hang at the beforeAll hook for me, specifically here.

After some investigation, it just seems unable to connect:

KnexTimeoutError: Knex: Timeout acquiring a connection. The pool is probably full. Are you missing a .transacting(trx) call?

I'm not sure what it's trying to connect to. I tried running the usual app postgres docker container just in case, but to no avail. Is it expecting a local postgres server or something by any chance?

thomasheartman · 2024-03-20T14:48:20Z

Oh, sorry, yeah, I think you're right. I've just been running these with a dockerized postgres running always, so I didn't really think about that. I think it will attempt to connect with the default Unleash credentials etc. I'm not sure whether it needs the schema to exist etc. or if it will create it itself.

If it helps, here's the output of my docker ps:

CONTAINER ID   IMAGE      COMMAND                  CREATED         STATUS         PORTS                    NAMES
70cdab965a8f   postgres   "docker-entrypoint.s…"   11 months ago   Up 5 minutes   0.0.0.0:5432->5432/tcp   postgres

I can have a closer look tomorrow if it doesn't work when just spinning up a local version.

00Chaotic · 2024-03-21T10:08:46Z

That's pretty much the same as my docker ps, although obviously that doesn't tell us much about the state of the db.

I tried tearing down my postgres container and running a fresh container with the instructions in CONTRIBUTING.md, but it still seems unable to connect. That file was last updated 10 months ago so not sure if it's still up-to-date, or it could be a different issue altogether.

Edit: Tried running the app before the tests, after freshly creating the container in case the migrations or similar made any difference, but still seeing the same error

thomasheartman · 2024-03-21T10:31:40Z

@00Chaotic Hey, sorry for causing you problems! I sat down to figure it out. In short, this worked for me:

docker run --name unleash-postgres -p 5432:5432 -e POSTGRES_USER=unleash_user -e POSTGRES_PASSWORD=password -e POSTGRES_DB=unleash_test -d postgres:15

Importantly, we:

create the expected unleash_user with the expected password.
Create the expected unleash_test schema

thomasheartman · 2024-03-21T10:33:10Z

This is sourced from checking the values in src/test/e2e/helpers/database-config.ts, by the way

00Chaotic · 2024-03-22T03:24:37Z

No worries about the trouble, thanks for being so patient haha.

It's working now with the latest command. I'll leave it to you to decide whether any test-related docs need to be created/updated to indicate the correct container setup for the tests.

I needed to remove the .end() statements from each test to get mine to succeed but I'm not sure if this is another weird Windows vs Unix issue.

Regarding the first test failing, it seems the admin user does not exist so it throws a No user found error when it tries to log in as admin.

Edit: Nevermind, the user does seem to exist in the database so I'm not sure why it isn't being found by the app.

thomasheartman · 2024-03-22T06:57:11Z

It's working now with the latest command. I'll leave it to you to decide whether any test-related docs need to be created/updated to indicate the correct container setup for the tests.

I think adding that would be helpful, but that shouldn't be on you. I'll see what I can do, though.

I needed to remove the .end() statements from each test to get mine to succeed but I'm not sure if this is another weird Windows vs Unix issue.

Sorry, what .end() statements are you talking about? What file is this?

Regarding the first test failing, it seems the admin user does not exist so it throws a No user found error when it tries to log in as admin.

Edit: Nevermind, the user does seem to exist in the database so I'm not sure why it isn't being found by the app.

Yeah, it should exist, I think. Not sure why it's not finding it, though, but it's probably debuggable.

Do you wanna make a new PR for this, by the way, so that we can look at the set of changes and discuss it in context? ☺️

There was some confusion on how to run the e2e tests in #6447, so I'm adding a small section to CONTRIBUTING.md to make it clearer.

00Chaotic · 2024-03-23T13:03:32Z

Sorry, what .end() statements are you talking about? What file is this?

Apologies, I think I added that statement in and then didn't realise it was my own addition haha, it doesn't seem to be present in the code you provided.

Do you wanna make a new PR for this, by the way, so that we can look at the set of changes and discuss it in context? ☺️

I was planning on raising a PR once we had the functionality+tests working so that the discussion on that PR would be about improvements rather than about just getting it working in the first place, but yes I can raise a PR if we want to move discussion to that 🙂

I'll be a little busy for the next few days though so I'll be able to get to it by the middle or end of next week.

thomasheartman · 2024-03-25T08:01:06Z

I was planning on raising a PR once we had the functionality+tests working so that the discussion on that PR would be about improvements rather than about just getting it working in the first place, but yes I can raise a PR if we want to move discussion to that 🙂

I'll be a little busy for the next few days though so I'll be able to get to it by the middle or end of next week.

Sure thing; whatever works for you! I'll be out for the rest of this week and until next Tuesday anyway, so there's no rush.

00Chaotic · 2024-03-31T10:56:31Z

I've been poking around but haven't been able to find a solution for the successful admin login test case. It did seem to give the correct output one time but I hadn't actually changed anything, and then it went back to giving the No user found error so I'm wondering if it's an async/timing issue.

I tried commenting the other test cases out in case they were messing with this one but it doesn't seem to have made a difference.

Also, I needed to add a 20sec timeout to the beforeAll hook so that it would actually run the tests, have you experienced this as well?

I've updated the branch with my current WIP code for the two tests that are working and the one that isn't, in case you'd like to take a look. Hopefully you have more luck than I have had thus far 😅

thomasheartman · 2024-04-04T07:57:05Z

It did seem to give the correct output one time but I hadn't actually changed anything, and then it went back to giving the No user found error so I'm wondering if it's an async/timing issue.

Huh, that sounds strange. I'll see if anything jumps out at me. Let me have a look and get back to you later today, yeah?

Also, I needed to add a 20sec timeout to the beforeAll hook so that it would actually run the tests, have you experienced this as well?

No, this doesn't sound familiar to me. I'm not sure what's going on there?

) There was some confusion on how to run the e2e tests in #6447, so I'm adding a small section to CONTRIBUTING.md to make it clearer.

thomasheartman · 2024-04-04T12:00:27Z

So looking at this a bit:

I don't need the timeout to run the tests. They run just fine without it. The timeout should only be necessary to stop tests that are stuck, right?
I can replicate the failure, but I also found a way around it that shows that it should work. Let's explore that a bit more.

In loginDemoAuthDefaultAdmin, if I add an await this.store.getAll() before I query for the admin user (say, at the top of the function), I get the results I expect and the test passes. However, the getAll call doesn't always return any users. This makes it seem like the stores haven't initialized fully yet, but the extra time spent calling getAll before the query gives it the time it needs.

However, there's also this in the console:

A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.

I think we've seen that happen due to missing awaits. That would also explain why something isn't set up correctly.

Does that give you enough to go on to debug this further?

I also noticed that I made a mistake previously, assuming that you'd get a 403 if you tried to log in with an invalid email. Turns out you don't. But the response should be the same as with any other non-email. So I think that test should be changed to something like this:

test('if the flag is disabled, using `admin` should have the same result as any other invalid email', async () => {
    const app = await getApp(false);

    const nonAdminUsername = 'not-an-email'
    const adminUsername = 'admin'

    const nonAdminUser = await app.request
        .post(`/auth/demo/login`)
        .send({ email: nonAdminUsername })

    const adminUser = await app.request
        .post(`/auth/demo/login`)
        .send({ email: adminUsername })

    // verify that the status is the same
    expect(nonAdminUser.status).toBe(adminUser.status);

    for (const user of [nonAdminUser, adminUser]) {
        // verify that the body has the same structure
        expect(user.body).toMatchObject({
            error: expect.stringMatching(/^Could not sign in with /)
        })
    }
});

That also means that you can remove this if statement from the middleware:

            if (!authentication.authDemoAllowAdminLogin && email === 'admin') {
                return res.status(403)
                    .json({ error: `Admin login not enabled for demo auth type`})
                    .end;
            }

00Chaotic · 2024-04-05T09:32:24Z

Regarding needing the timeout in the beforeAll(), it seems to take ~17sec to initialise for me so I get the below error without the timeout:

thrown: "Exceeded timeout of 10000 ms for a hook.
    Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

       7 | let stores: IUnleashStores;
       8 |
    >  9 | beforeAll(async () => {
         | ^
      10 |     db = await dbInit('demo_auth_serial');
      11 |     stores = db.stores;
      12 | });

      at Object.beforeAll (src/lib/middleware/demo-authentication.test.ts:9:1)

Since you don't have this issue, it may be something weird with my setup so I'll remove the timeout on the next commit and we can see if it passes the PR checks once raised.

I looked into the worker process warning in the console that you've mentioned but I haven't been able to reproduce it, so we can do the same as the timeout and just see if the PR checks pass for that.

Thanks for looking into the failing test case, I was able to replicate the behaviour you noticed with the timing so you're right, it's most likely that the admin user hasn't yet been populated when the test case is run. We could just add a small timeout to delay that specific test case but that feels like a bit of a hacky workaround.

Does the current testing implementation include any way to check that the stores are initialised before running the test cases? I was thinking of checking the /health endpoint for that specific test case but I'm assuming that only guarantees the app is healthy, not necessarily that the stores have been populated.

thomasheartman · 2024-04-08T10:33:40Z

Since you don't have this issue, it may be something weird with my setup so I'll remove the timeout on the next commit and we can see if it passes the PR checks once raised.

Yeah, maybe. Mine run immediately, so I'm not sure what's going on.

I looked into the worker process warning in the console that you've mentioned but I haven't been able to reproduce it, so we can do the same as the timeout and just see if the PR checks pass for that.

That's also interesting. Again I think we've seen that one previously when there's an unawaited expression somewhere. But I might be wrong. That said, I don't see it now for some reason 🤷🏼 You can probably ignore that, then.

We could just add a small timeout to delay that specific test case but that feels like a bit of a hacky workaround.
Does the current testing implementation include any way to check that the stores are initialised before running the test cases? I was thinking of checking the /health endpoint for that specific test case but I'm assuming that only guarantees the app is healthy, not necessarily that the stores have been populated.

We could add the delay, but as you mentioned that is hacky and I'm not sure it would work. As far as I'm aware, there's no specific way of checking the stores are initialized correctly, but it shouldn't be necessary. This isn't the only e2e test we have, and it's not like the others don't use the stores. I'm not sure what's going on.

However, in other hacks that might fix it: try putting that first test last in the file. Doing that on my end seems to make it pass consistently.

Again, it's not ideal, but this feels like it's on our plate to deal with and not yours 😅

00Chaotic · 2024-04-09T12:22:02Z

Haha that's all good, thanks for your patience this far. Seems the tests have been more of a headache than the actual code implementation 😅

The test case seems to be working for me too if I move it to the end of the file as you suggested.

Regarding the below:

I also noticed that I made a mistake previously, assuming that you'd get a 403 if you tried to log in with an invalid email. Turns out you don't. But the response should be the same as with any other non-email. So I think that test should be changed to something like this:

test('if the flag is disabled, using `admin` should have the same result as any other invalid email', async () => {
    const app = await getApp(false);

    const nonAdminUsername = 'not-an-email'
    const adminUsername = 'admin'

    const nonAdminUser = await app.request
        .post(`/auth/demo/login`)
        .send({ email: nonAdminUsername })

    const adminUser = await app.request
        .post(`/auth/demo/login`)
        .send({ email: adminUsername })

    // verify that the status is the same
    expect(nonAdminUser.status).toBe(adminUser.status);

    for (const user of [nonAdminUser, adminUser]) {
        // verify that the body has the same structure
        expect(user.body).toMatchObject({
            error: expect.stringMatching(/^Could not sign in with /)
        })
    }
});

That also means that you can remove this if statement from the middleware:

            if (!authentication.authDemoAllowAdminLogin && email === 'admin') {
                return res.status(403)
                    .json({ error: `Admin login not enabled for demo auth type`})
                    .end;
            }

Would it not be more informative to give the 403 if admin is used with the flag disabled, rather than the same message that would be used for an invalid email?

Also, since we've now fixed any issues not actually related to the implementation of this change (darn pesky tests), I'll go ahead and raise a PR and we can continue discussion there regarding what the change should/shouldn't include.

00Chaotic · 2024-04-09T12:37:32Z

Created PR #6808

Feel free to close this PR if there is nothing further that should be discussed here instead of the new PR

thomasheartman · 2024-04-10T08:02:31Z

Haha that's all good, thanks for your patience this far.

Of course; happy to! Thanks for contributing 🙌🏼

Seems the tests have been more of a headache than the actual code implementation 😅

Yeah, that happens quite a bit, I'm afraid, but that's how it is. It's especially tricky with e2e tests like this.

Would it not be more informative to give the 403 if admin is used with the flag disabled, rather than the same message that would be used for an invalid email?

This is a good point. The reason I suggested a 403 initially was that that's usually what happens if you try to log in with invalid credentials ... But I also think I might have mixed it up with 401, sooo 😅

Anyway: should we return a special response when you use admin? I'm inclined to say that at the very least, the response code should be the same: your login failed, after all. And with that, I feel like we can avoid complexity if we don't special-case this. I don't think gain much by giving a special message there and I think it'll just lead to code that's harder to read.

If you know about this feature (because you've read the docs or the release notes or whatever), then you'll probably understand why you're not able to log in with admin (or indeed, why you are able to log in). And if you don't, then you probably wouldn't try it in the first place.

Of course, I'd be happy to hear further arguments against this if you think the other way is better.

Created PR #6808

Sick! I'll go take a look and we can continue any discussions (or start new ones) there.

This PR introduces a configuration option (`authentication.demoAllowAdminLogin`) that allows you to log in as admin when using demo authentication. To do this, use the username `admin`. ## About the changes The `admin` user currently cannot be accessed in `demo` authentication mode, as the auth mode requires only an email to log in, and the admin user is not created with an email. This change allows for logging in as the admin user only if an `AUTH_DEMO_ALLOW_ADMIN_LOGIN` is set to `true` (or the corresponding `authDemoAllowAdminLogin` config is enabled).  Closes #6398 ### Important files [demo-authentication.ts](https://github.com/Unleash/unleash/compare/main...00Chaotic:unleash:feat/allow_admin_login_using_demo_auth?expand=1#diff-c166f00f0a8ca4425236b3bcba40a8a3bd07a98d067495a0a092eec26866c9f1R25) ## Discussion points Can continue discussion of [this comment](#6447 (comment)) in this PR. --------- Co-authored-by: Thomas Heartman <[email protected]>

feat: Add configurable default email for initial admin user

722f6f4

00Chaotic requested a review from thomasheartman as a code owner March 6, 2024 11:35

github-actions bot assigned 00Chaotic Mar 6, 2024

codescene-delta-analysis bot reviewed Mar 6, 2024

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend March 6, 2024 11:38 View deployment

thomasheartman reviewed Mar 6, 2024

View reviewed changes

sighphyre reviewed Mar 6, 2024

View reviewed changes

ivarconr reviewed Mar 6, 2024

View reviewed changes

00Chaotic mentioned this pull request Mar 13, 2024

Initial/default admin user cannot be accessed when using demo authentication type #6398

Closed

thomasheartman added a commit that referenced this pull request Mar 22, 2024

docs: explicitly call out how to run e2e tests in CONTRIBUTING.md

99a3c94

There was some confusion on how to run the e2e tests in #6447, so I'm adding a small section to CONTRIBUTING.md to make it clearer.

thomasheartman mentioned this pull request Mar 22, 2024

docs: explicitly call out how to run e2e tests in CONTRIBUTING.md #6672

Merged

thomasheartman added a commit that referenced this pull request Apr 4, 2024

docs: explicitly call out how to run e2e tests in CONTRIBUTING.md (#6672

99a32a4

) There was some confusion on how to run the e2e tests in #6447, so I'm adding a small section to CONTRIBUTING.md to make it clearer.

00Chaotic mentioned this pull request Apr 9, 2024

Feat/allow admin login using demo auth #6808

Merged

thomasheartman closed this Apr 10, 2024

	`Creating default user '${username}', password '${password}', and email '${email}'`,
	`Creating default user '${username}' with password '${password}' and email '${email}'`,

feat: Add configurable default email for initial admin user #6447

feat: Add configurable default email for initial admin user #6447

Conversation

00Chaotic commented Mar 6, 2024 • edited Loading

vercel bot commented Mar 6, 2024 • edited Loading

vercel bot commented Mar 6, 2024

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

🚩 Declining Code Health (highest to lowest):

codescene-delta-analysis bot Mar 6, 2024

Choose a reason for hiding this comment

Why does this problem occur?

thomasheartman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivarconr Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveleek Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

daveleek commented Mar 6, 2024

thomasheartman commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivarconr commented Mar 6, 2024

00Chaotic commented Mar 7, 2024

thomasheartman commented Mar 13, 2024 • edited Loading

thomasheartman commented Mar 13, 2024

thomasheartman commented Mar 13, 2024

00Chaotic commented Mar 14, 2024 • edited Loading

thomasheartman commented Mar 14, 2024

00Chaotic commented Mar 14, 2024 • edited Loading

00Chaotic commented Mar 14, 2024

thomasheartman commented Mar 14, 2024

thomasheartman commented Mar 14, 2024

00Chaotic commented Mar 14, 2024

thomasheartman commented Mar 14, 2024

00Chaotic commented Mar 15, 2024

thomasheartman commented Mar 15, 2024 • edited Loading

00Chaotic commented Mar 16, 2024

thomasheartman commented Mar 18, 2024

00Chaotic commented Mar 19, 2024

thomasheartman commented Mar 19, 2024 • edited Loading

thomasheartman commented Mar 19, 2024 • edited Loading

00Chaotic commented Mar 20, 2024

thomasheartman commented Mar 20, 2024

00Chaotic commented Mar 21, 2024 • edited Loading

thomasheartman commented Mar 21, 2024 • edited Loading

thomasheartman commented Mar 21, 2024

00Chaotic commented Mar 22, 2024 • edited Loading

thomasheartman commented Mar 22, 2024

00Chaotic commented Mar 23, 2024

thomasheartman commented Mar 25, 2024

00Chaotic commented Mar 31, 2024

thomasheartman commented Apr 4, 2024

thomasheartman commented Apr 4, 2024

00Chaotic commented Apr 5, 2024

thomasheartman commented Apr 8, 2024

00Chaotic commented Apr 9, 2024

00Chaotic commented Apr 9, 2024

thomasheartman commented Apr 10, 2024

00Chaotic commented Mar 6, 2024 •

edited

Loading

vercel bot commented Mar 6, 2024 •

edited

Loading

ivarconr Mar 6, 2024 •

edited

Loading

daveleek Mar 6, 2024 •

edited

Loading

thomasheartman commented Mar 13, 2024 •

edited

Loading

00Chaotic commented Mar 14, 2024 •

edited

Loading

00Chaotic commented Mar 14, 2024 •

edited

Loading

thomasheartman commented Mar 15, 2024 •

edited

Loading

thomasheartman commented Mar 19, 2024 •

edited

Loading

thomasheartman commented Mar 19, 2024 •

edited

Loading

00Chaotic commented Mar 21, 2024 •

edited

Loading

thomasheartman commented Mar 21, 2024 •

edited

Loading

00Chaotic commented Mar 22, 2024 •

edited

Loading