fix(frontend): give a real error when the viewer has no environment key - #973
Open
omlahore wants to merge 1 commit into
Open
fix(frontend): give a real error when the viewer has no environment key#973omlahore wants to merge 1 commit into
omlahore wants to merge 1 commit into
Conversation
Eight call sites read the viewer's environment key as `data.environmentKeys[0]` and destructure it immediately. That list is a queryset filtered by environment and member, so an empty result is a normal outcome rather than an error state, and destructuring it throws "Cannot destructure property 'wrappedSeed' of 'undefined'" from inside an async callback. The user is told nothing, and at the sites that do not catch, the dialog just stops responding. An empty list is reachable because the resolver guards on `user_can_access_app` while filtering keys by environment and member: - CreateEnvironmentMutation provisions `admin_keys` only, so a non-admin member with app access holds no key for an environment created after they were added. - AddAppMemberMutation takes a client-supplied `env_keys` list, so a member can be granted app access for a subset of environments. Add `requireEnvironmentKey` next to the other environment crypto helpers and use it at each site, so the failure names the environment and says what to do about it. Behaviour is otherwise unchanged: the sites that threw still throw, with a message that means something.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The problem
Eight call sites read the viewer's own environment key as
data.environmentKeys[0]and destructure it straight away:When the list is empty this throws
Cannot destructure property 'wrappedSeed' of 'undefined'from inside an async callback. The user gets nothing useful, and at the sites without acatchthe dialog simply stops responding.Why an empty list is a normal outcome
resolve_environment_keysguards on app access but filters keys by environment and member:So holding app access does not imply holding a key for a given environment. Two paths reach that state:
CreateEnvironmentMutationprovisionsadmin_keysonly, so a non-admin member with app access holds no key for an environment created after they were added.AddAppMemberMutationtakes a client-suppliedenv_keyslist and derivesnew_env_idsfrom it, so a member can be granted app access for a subset of environments.The change
Adds
requireEnvironmentKeyalongside the other environment crypto helpers and uses it at each site:The failure now names the environment and says what to do:
Behaviour is otherwise unchanged. The sites that threw still throw, with a message that means something. No
catchblocks were added or removed.Checks
tsc --noEmitonfrontendis clean.eslinton the changed files is clean.prettier --checkflags 5 of these files, but they fail identically on an unmodifiedmain, so I left the formatting alone rather than bury the change in an unrelated reformat.