Feat/allow admin login using demo auth #6808
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@00Chaotic is attempting to deploy a commit to the unleash-team Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Thanks for taking the time to work out a solution and implementing it 🙌🏼 This looks pretty good to me, but I've requested changes due to the admin return code that we've discussed before. Once that's changed (or if you convince me that this really is the right way), then I'd be happy to approve it.
As for why I think not returning a special case for admin when the flag is disabled, refer to my last comment on the other PR. (I'm sure you've seen it, but it's also for anyone else reading this later 💁🏼 )
I've also added some small suggestions to the test titles to make it clearer what exactly they are testing.
|
Thanks for the review, I've made the requested changes to the test case naming. Regarding the error code:
|
Of course! Thanks for making the changes 😄
Yup, these are good points. On 401 vs 403: I did go and read up on when to use 401 vs 403, and I got a sense that people generally recommended 403 for areas you're not allowed to enter with your credentials. In other words, "I know who you are, but you don't have access here". On the other hand, 401 was recommended for when the user isn't authenticated. You could say that this case is "special", because in a sense, the credentials are correct, but "you can't come in here right now". On the other hand, we also don't let you authenticate, so the server doesn't really know who you are 💁🏼 To quote MDN's doc on 401 (with added emphasis):
And if that flag isn't enabled, then those authentication credentials aren't valid. Which brings us back to the 400. If the flag isn't enabled, then 'admin' isn't any different (and thus shouldn't be treated any differently, I think) from any other non-email address. And the established pattern is that Unleash gives you a 400 in that case. I think you could make an argument that it should be 401, but the 400 makes sense because you've not provided the data that Unleash asked for. But then again, I'm not an expert on HTTP status codes, and this is all just my understanding. However, as the arbiter here, I'm going to say I remain unconvinced that we should create a special case for 'admin' when the flag is disabled. With the points you provided, I think 400, 401, and (maybe to a lesser extent?) 403 are all valid options. But Unleash has an established precedent of returning 400 if the email isn't an email, and I don't see this case as being so different that it warrants special-casing. This is both to keep the interface consistent (people who don't use the flag shouldn't need to care whether someone provides "admin" or "bogus"; they're both invalid) and to keep the code simpler. So my request still stands. I think that with the flag disabled, nothing should change for existing users and people who don't use the flag. 'admin' should be treated exactly as any other invalid email address. Does that sound agreeable to you? |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
| let user: IUser; | ||
|
|
There was a problem hiding this comment.
❌ New issue: Complex Method
demoAuthentication has a cyclomatic complexity of 12, threshold = 9
|
Yep, at the end of the day it is your product so if you feel the I've pushed the changes along with some linting fixes, unfortunately locally it messes up with the line endings and suggests changes to 2000+ files so I'll just see what the PR check says |
|
Super! Thanks for your hard work!
Yeah, linting appears to be failing. Weird that it messes up line endings locally for you. Unfortunately, we don't have automated lint fixing in CI, but there's not that many errors. Maybe try just running the formatter on the three specific files that fail? You can probably target specific files with the CLI, as mentioned in the docs? |
|
@thomasheartman not sure if the build check is hanging, seems to still be waiting for a status after ~24hrs since the last change |
|
Thanks for reporting in! I think you're right there. Running it now 🙋🏼 |
There was a problem hiding this comment.
Found another linting issue, but I took the liberty of fixing it myself. (Oh, don't worry about commit history, by the way; we'll squash it down before merging anyway).
Tests run in CI now, but one of them is failing.
Also, got a suggestion for renaming the new variable on the authentication object, with reasoning in the comment. I'd love to hear your thoughts on that too.
src/lib/create-config.test.ts
Outdated
|
|
||
| const config = createConfig({}); | ||
|
|
||
| expect(config.authentication.authDemoAllowAdminLogin).toBeTruthy(); |
There was a problem hiding this comment.
This test is failing for some reason. Does it run correctly locally for you?
There was a problem hiding this comment.
Okay this is weird, the process.env.AUTH_DEMO_ALLOW_ADMIN_LOGIN isn't being evaluated correctly. It evaluates to 'true' if I print it in this test case but if I add a console.log() in parseEnvVarBoolean() which is used here, it prints undefined.
Strange because a lot of the other tests in this file do the exact same thing and yet it correctly parses those env vars 🤔
Can't really be a timing/race issue either since those operations aren't async.
Is there any behind-the-scenes env var processing I'm not aware of that may be inadvertently affecting this variable?
Edit: It's also true in the parent createConfig() function so I have no idea why it magically changes in the parse function. However, I have noticed that there are no other boolean env vars being set in that config test file. What are the chances this is an existing or larger issue that has only been caught now that we actually have a boolean env var in the tests?
There was a problem hiding this comment.
Hmm, so this is a little tricky, but it is actually kind of a timing issue.
The thing is, most of the other tests in that file have corresponding functions in createConfig, but defaultAuthentication is defined as a variable. So it does the parsing of the env variables when that file first gets loaded, which happens before you set the process variable.
You can try this out by setting any of the other auth variables to a different value (boolean or otherwise).
So this isn't really an issue for Unleash itself. It makes testing a little trickier, but only in this specific case.
We could make it so that the auth config is also loaded through a function, but I'm a little hesitant to do that. It might have further security repercussions if you can dynamically change the environment and thus the auth options of Unleash after it starts. It seems a little far-fetched, but I'd rather not change it.
So how do we deal with this, then? It feels a little weird, but I think I'd actually just remove that test.
But I'd actually like to hear @sighphyre's take here: is it safe to load auth options dynamically or does it pose potential risks? Unleash ever only does it on startup, so I don't see how it's really gonna be an issue, but I'm not a security expert. And if not, is this test low-stakes enough that leaving it out is fine?
There was a problem hiding this comment.
Happy to wait for their input.
I'd be hesitant to remove the test since this might be our only way of knowing if the true case ever stops working in future, whereas for the false case we still have the default config case that will include this flag.
I don't really have a better suggestion though so if you both agree that this is the best way forward then we can take this route.
There was a problem hiding this comment.
You're right: we won't have a test that checks whether you can set it via environment variables (which it also seems like we don't explicitly for a lot of the other config variables). But we do test that you can turn it on via config in the e2e test file, so it's not untested.
So the only thing that test is actually doing is checking that we parse the env var correctly. Which definitely isn't worthless, but feels like it's unlikely to break. If we break env var parsing, then the other tests that load env vars dynamically will yell at us. If we get the name wrong, well, we'd have to keep supporting the wrong name, then. But a test won't really protect us against that unless we go and update the exposed env var directly, which would be a breaking change and thus not likely to happen.
So in short: I think it'd be nice if it worked, and if loading it dynamically is cool, then that's great! However, I'm also not too bothered if it turns out to be better to leave this one case out.
There was a problem hiding this comment.
But I'd actually like to hear @sighphyre's take here: is it safe to load auth options dynamically or does it pose potential risks? Unleash ever only does it on startup, so I don't see how it's really gonna be an issue, but I'm not a security expert. And if not, is this test low-stakes enough that leaving it out is fine?
I think if you've lost control of your environment and an attacker can inject something in there then you have much, much bigger problems on your hands. I don't see any harm in moving that to being loaded later in the startup sequence though, if only for testing purposes. It should still only be loaded once when this is invoked.
So weirdly enough I don't really have strong feelings about what we do here. Mostly because demo auth isn't a core feature and the most dangerous thing that can happen here is that is gets accidentally turned on. So I have strong feelings about adjacent things - that demo auth remains off by default and that that test is clear but that seems to be covered by the test below the one in question.
I think @thomasheartman is right here, this is covered enough by the e2e tests. So it largely depends on how strongly you feel about it, @00Chaotic. I'd lean in the direction of removing the test here and getting this merged because gosh this conversation has gone on for a long time and PRs that stay open for a long time get worse and worse to try merge. I think this is good enough to merge in it's current state.
I don't see harm in restructuring the way this gets loaded to later if you think the quality would be improved by a more isolated test though.
There was a problem hiding this comment.
Thanks for your feedback, I'm happy to remove the tests. I agree about just trying to get it merged at this point, it has definitely been messy with long discussions in multiple PRs haha.
I'll go ahead and remove the test case, and we can merge it in once all the checks pass.
|
Haven't had a chance to look at this for a few days but just saw the email for the failing linting issue (apologies, rebased and force pushed which probably wiped your changes). I agree with the name change and will get that pushed, and look into the failing test 👍 |
As a side-note unrelated to this PR, the squash vs merge debate seems to be a long-standing one, so I'm curious as to your reasoning, if any, for using squash merges (if that's what you meant as opposed to manually squashing the commits and then merging). There are multiple articles arguing for both sides but this one is one I've come across a couple times. At my work we're moving away from squash merging and more towards manually merging individual commits (if needed), and then doing a normal merge into the target branch. Not that I'm pushing for it to be done here, but just food for thought 🙂 |
Ah, yes; one of the ever-present developer debates! Honestly, it makes things easy and quick for us, and that's my main motivation, at least. I remember there was a pretty clear reasoning when we set that as the default, but I can't remember it exactly. I read the article, and while I think the author has some good points, it also does exactly what it says closer to the conclusion:
I think it very much comes down to your team's workflow. PRs here are getting smaller and smaller, and being able to merge the entire content with one clear message is nice 🤷🏼 Sure, we could rebase, rewrite commit messages, etc, but I'm not convinced it's worth it, personally. That said, I'd love it if github made it easier to split a PR up into multiple PRs or to structure changes into a series of commits. Similarly, in git, if you have a series of patches, I'd love to be able to say "actually, take lines 57 through 68 in file A and lines 15, 17, and 25 in file B and but them in a different branch/commit/(or indeed github's PR)" even when those changes have been made in three different commits. I'm sure you can do it, but I'm not sure it'll be quick or easy. So eh 🤷🏼 I used to believe strongly in one direction, but then someone convinced me otherwise. Now I think all (most?) approaches have their merit, and when it comes down to which is the right one for your team: the answer is the ol' adage "it depends". |
Remove test that was failing. The variable is loaded statically at startup and dynamically, so this test can't pass unless we change other things.
|
@00Chaotic I've made the change to the tests that we discussed in the thread (and removed that one test) and merged the PR. Thanks a whole bunch for your contribution and for the discussions we had along the way 🙌🏼 |
This PR adds docs for the new `demoAllowAdminLogin` option, including how to use it and what it does. Documents the changes introduced in #6808
|
Ah thanks @thomasheartman, just saw your latest comment. Glad to have the issue closed. I really enjoyed working together, thanks for your patience and perseverance with this haha. Wishing you all the best with Unleash 🙏 |
About the changes
The
adminuser currently cannot be accessed indemoauthentication 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 anAUTH_DEMO_ALLOW_ADMIN_LOGINis set totrue(or the correspondingauthDemoAllowAdminLoginconfig is enabled).Closes #6398
Important files
demo-authentication.ts
Discussion points
Can continue discussion of this comment in this PR.