Page MenuHomePhabricator

[keyserver] Instruct to restart nix if login / register fails in the new flow
ClosedPublic

Authored by inka on Mar 7 2024, 3:42 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 10, 12:12 AM
Unknown Object (File)
Fri, Apr 26, 1:35 AM
Unknown Object (File)
Thu, Apr 25, 8:10 AM
Unknown Object (File)
Thu, Apr 25, 8:10 AM
Unknown Object (File)
Thu, Apr 25, 8:10 AM
Unknown Object (File)
Thu, Apr 25, 8:10 AM
Unknown Object (File)
Thu, Apr 25, 8:10 AM
Unknown Object (File)
Thu, Apr 25, 8:06 AM
Subscribers

Details

Summary

issue: ENG-7137
If the dev is using the new multikeyserver flow, they had provided some credentials during when nix was run. If registration with these credentials failed, they neeed to rerun nix, so that they can provide new credentials, but also so that the db gets fixed.

Test Plan

Tested that if registration fails, and the flag is set, the warning is printed

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Mar 7 2024, 4:03 AM
keyserver/src/user/login.js
149 ↗(On Diff #37918)

Is the === true part really necessary?

150–155 ↗(On Diff #37918)
151 ↗(On Diff #37918)

(Might need to split lines again to maintain 80 chars)

152 ↗(On Diff #37918)

Should we only print this log in dev mode?

Should we only print this log in dev mode?

It looks like the way we check if this is dev mode is by checking process.env.NODE_ENV === 'development'. But docker keyserver prints undefined as the value of process.env.NODE_ENV. Would checking process.env.NODE_ENV !== 'production' make sense? Is the real production keyserver set up to have process.env.NODE_ENV == 'production'?

But docker keyserver prints undefined as the value of process.env.NODE_ENV

Hmm... we shouild probably should avoid this ambiguous situation, and make sure we always specify NODE_ENV in the Docker environment. Can you create a task?

Would checking process.env.NODE_ENV !== 'production' make sense? Is the real production keyserver set up to have process.env.NODE_ENV == 'production'?

It doesn't look like I have anything specified in the .env file. If you're correct that the default keyserver is to have undefined, then I'm guessing my keyserver in production also has undefined. If it's helpful, I can do some additional testing on this.

But docker keyserver prints undefined as the value of process.env.NODE_ENV

Hmm... we shouild probably should avoid this ambiguous situation, and make sure we always specify NODE_ENV in the Docker environment. Can you create a task?

Would checking process.env.NODE_ENV !== 'production' make sense? Is the real production keyserver set up to have process.env.NODE_ENV == 'production'?

It doesn't look like I have anything specified in the .env file. If you're correct that the default keyserver is to have undefined, then I'm guessing my keyserver in production also has undefined. If it's helpful, I can do some additional testing on this.

In general the keyserver run in Docker is always a production keyserver. And I think we want this warning to show up, since it means that the keyserver is set up to use the multikeyserver flows, but something went wrong and some action is required to fix this state.

we shouild probably should avoid this ambiguous situation, and make sure we always specify NODE_ENV in the Docker environment

Since the Docker keyserver is always a prodution keyserver, does that make sense? I feel like we should either decide that production keyservers don't have the value set up, or specify it as 'production' for all production keyservers

In general the keyserver run in Docker is always a production keyserver. And I think we want this warning to show up, since it means that the keyserver is set up to use the multikeyserver flows, but something went wrong and some action is required to fix this state.

I don't think it makes sense to print this particular warning language in the Docker environment, since we aren't using nix develop there. I'm open to printing a warning, but I think it should make sure where it's printed.

I feel like we should either decide that production keyservers don't have the value set up, or specify it as 'production' for all production keyservers

I'm okay with either solution, but if you opt for the first one ("decide that production keyservers don't have the value set up"), we should go through places where we check that NODE_ENV is 'production'. In those case, we likely need to be checking that NODE_ENV is not set OR it's 'production'. Similarly, we should consider places where we check that NODE_ENV is NOT 'production', in which case we should check whether NODE_ENV is set AND it's NOT 'production'.

git grep NODE_ENV | grep production could be a good place to start this search:

  • The process.env.NODE_ENV !== 'production' check in createAsyncMigrate can arguably be removed entirely... it seems superfluous
  • I'm not sure about the process.env.NODE_ENV check in web/push-notif/service-worker.js. This appears to occur on web, but our Webpack config seems to make sure that NODE_ENV is always either 'production' or 'development'
  • The other results don't appear to be relevant

I created a task to update NODE_ENV usage: ENG-7195

I don't think it makes sense to print this particular warning language in the Docker environment, since we aren't using nix develop there. I'm open to printing a warning, but I think it should make sure where it's printed.

I see, I will only print the "nix" part in dev mode then

Print instructions for nix only in dev mode

ashoat added inline comments.
keyserver/src/user/login.js
163 ↗(On Diff #38020)

Production keyserver can still be in non-Docker, right? Can the language also give guidance for the non-Docker environment? ("re-enter nix develop")

This revision is now accepted and ready to land.Mar 12 2024, 7:29 AM