Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/allow admin login using demo auth #6808
Feat/allow admin login using demo auth #6808
Changes from 1 commit
f9016fe
4702019
175d2f5
5e2b432
f9a44f3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This test is failing for some reason. Does it run correctly locally for you?
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.
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 aconsole.log()
inparseEnvVarBoolean()
which is used here, it printsundefined
.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 parentcreateConfig()
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
, butdefaultAuthentication
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thefalse
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
❌ New issue: Complex Method
demoAuthentication has a cyclomatic complexity of 12, threshold = 9
Suppress