Skip to content
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

Grant first user on a new installation super admin access #2520

Merged
merged 2 commits into from
May 17, 2024

Conversation

bttf
Copy link
Collaborator

@bttf bttf commented May 14, 2024

RE: Thread on a previous PR, we want to grant the first user to be created on a new installation with super admin access.

Changes

  • Updated createUser method to take single object argument
    • Add optional superAdmin property, defaults to false
  • We only grant super admin access in the /auth/firsttime endpoint
    • Endpoint used in situations where a 'new installation' is asserted (i.e., no existing orgs, no existing users)

Questions

  • Briefly had similar changes in the /auth/signup endpoint but reverted since it seemed like an impossible scenario. Could use confirmation on this and any add'l insights on what could have been missed.

@bttf bttf requested review from tzjames and jdorn May 14, 2024 21:44
Copy link

github-actions bot commented May 14, 2024

Your preview environment pr-2520-bttf has been deployed.

Preview environment endpoints are available at:

@@ -276,7 +276,9 @@ export async function postFirstTimeRegister(
});
}

const user = await createUser(name, email, password);
// grant the first user on a new installation super admin access
const user = await createUser({ name, email, password, superAdmin: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Digging into this endpoint more, isNewInstallation uses an in-memory cache, so it's possible in a horizontally scaled environment it could return true when there are actually already orgs/users.

Looking through the code, I don't think that in-memory cache is valuable. isNewInstallation is not called that often and it's only doing 2 efficient mongo lookups.

So, I'd recommend removing the caching layer to reduce the chance of bugs here and elsewhere.

Copy link
Collaborator Author

@bttf bttf May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the caching layer from isNewInstallation and the use of markInstalled entirely since the former method will check every time.

Copy link
Member

@jdorn jdorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll approve, but I do think we should make that change to isNewInstallation for extra safety

@bttf bttf merged commit bbf2285 into main May 17, 2024
3 checks passed
@bttf bttf deleted the adnan/super-admin-on-first-org branch May 17, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants