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

Feat: Default organization slug for LDAP/SAML #2000

Merged
merged 19 commits into from
Jun 24, 2024
Merged

Conversation

DanielHougaard
Copy link
Collaborator

@DanielHougaard DanielHougaard commented Jun 20, 2024

Description 📣

Main

We now have a way of setting the default slug as a part of the admin settings / server configuration. When the default org slug is set, a few things will happen:

  1. When attempting to log in with LDAP, the org slug input field will be hidden from the user, and the value will be pre-set to the default organization slug.
  2. When logging in with SAML and the default org slug is set, it will entirely skip the org slug input page, and take you straight to the SAML auth.
  3. If the default org slug is set, you'll never be asked to "select an organization". It will assume you're trying to log into the default organization and automatically select the default organization.

Technical:
Instead of storing the organization slug as text in the database, we're storing a reference to the organization itself by ID. This will allow cascading to work, and I generally believe we should always be keeping references instead of just plain text when we're keeping track of resources.

Minor:

  1. Fixed a type mismatch

Type ✨

  • Bug fix
  • New feature
  • Breaking change
  • Documentation

Tests 🛠️

# Here's some code block to paste some code snippets

@DanielHougaard DanielHougaard marked this pull request as ready for review June 20, 2024 16:26
@DanielHougaard DanielHougaard self-assigned this Jun 20, 2024
backend/src/db/schemas/super-admin.ts Outdated Show resolved Hide resolved
backend/src/services/super-admin/super-admin-service.ts Outdated Show resolved Hide resolved
@sheensantoscapadngan sheensantoscapadngan mentioned this pull request Jun 20, 2024
5 tasks
Copy link
Collaborator

@maidul98 maidul98 left a comment

Choose a reason for hiding this comment

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

I see what you are doing. User enters slug, and you take the slug and get it's id and save the id and when we get the admin config, we returns the id and the associated slug.

I just remembered, one of the friction points i observed watching customer do this is not knowing where the org slug is located at since it is their first time setting things up. I think it will be a better experience if they can just select a org from a drop down (we already have a API for getting users orgs) and then we send the id to the DB and save as is. When we fetch admin config, we can compute org slug as you do now. This way, saving is identical to before too

backend/src/services/super-admin/super-admin-service.ts Outdated Show resolved Hide resolved
@akhilmhdh akhilmhdh dismissed their stale review June 21, 2024 13:54

Looks good to me.

Dismissing since handing it over to @maidul98

Copy link
Collaborator

@maidul98 maidul98 left a comment

Choose a reason for hiding this comment

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

FYI, tried to load things up with a fresh DB and the migration that creates the instance row in the DB doesn't get created so the APIs throw error when it calls get config

backend/src/lib/fn/object.ts Outdated Show resolved Hide resolved
frontend/src/views/admin/DashboardPage/DashboardPage.tsx Outdated Show resolved Hide resolved
maidul98
maidul98 previously approved these changes Jun 21, 2024
Copy link
Collaborator

@maidul98 maidul98 left a comment

Choose a reason for hiding this comment

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

lgtm, let's definitely test in gamma since this not working will impact logins for enterprises

@maidul98 maidul98 merged commit 60cb420 into main Jun 24, 2024
6 of 9 checks passed
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

3 participants