-
Notifications
You must be signed in to change notification settings - Fork 109
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
Bed-4851 OIDC API Provider Registration #894
Conversation
…teractions to match new design
cmd/api/src/database/oidc.go
Outdated
return provider, CheckError(s.db.WithContext(ctx).Table("oidc_providers").Create(&provider)) | ||
// Create both the sso_providers and oidc_providers rows in a single transaction | ||
// If one of these requests errors, both changes will be rolled back | ||
err := s.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { |
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'm not the BIGGEST fan of intermingling the CreateSSOProvider and CreateOIDCProvider methods since the end-user may simply want to just create an oidc_provider
, for whatever reason. With our usecase, though, these tables are required to be in sync with each other so I felt comfortable doing this
…rly backfill saml_providers with the new sso_providers key. Added new enum type for sso_provider types
…ed a mapping from AuthProvider to the new SSOProviderType enum
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.
Did we want to also update the SAML create db op with a tx for creating SSO in this PR or wait for another one?
We still need to update the users table as well I think. Might be able to swap to sso_id vs 2 separate columns 🤔 That's likely worth a full separate PR
I was thinking another ticket, but there's no harm in throwing it in here as well while it's fresh in our minds |
…. Removed enum and replaced with integer for the sso_provider type
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.
Coming along solid! Sorry about the maybe scope creep question. Feel free to make a judgement call on that one.
…so_provider is deleted
Description
Modify the previously merged OIDC provider registration endpoint to support the new slug formatting, new sso_provider table, and move the URL to a new location to prevent collisions with existing endpoints.
Motivation and Context
After our syncs on 09/27 and 09/30, we decided to make some changes to the last PR in order to better support an agnostic API for both oidc & saml providers.
This PR addresses: BED-4851
How Has This Been Tested?
just bh-dev
to start up BHCE and ensure the migrations ranVerified a 201 response and that the data was inserted into the database with the correct formatting and referencing:
Screenshots (optional):
Types of changes
Checklist: