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

Bed-4852: SSO Provider Delete Endpoint #890

Merged
merged 41 commits into from
Oct 24, 2024
Merged

Conversation

mvlipka
Copy link
Contributor

@mvlipka mvlipka commented Sep 26, 2024

Description

Add a new endpoint to delete SSO Providers

Motivation and Context

This PR addresses: BED-4852

Adds the ability for admins to delete SSO Providers and any associated references to them

How Has This Been Tested?

Basic Test

I setup my database as followed:
image

Sent a request to
/api/v2/sso-providers/{sso_provider_id}
image

And received a 200 response back

Refreshing the tables in the database shows that the references were properly nulled out
image

Attempting to delete a non-existent sso provider:
image

Attempting to delete a SAML provider after replacing DeleteSAMLProvider with the DeleteSSOProvider method
image

After sending the request:
image

And the audit log with details:
image

Screenshots (optional):

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Database Migrations

Checklist:

@mvlipka mvlipka added the work in progress This pull request is a work in progress and should not be merged label Sep 26, 2024
cmd/api/src/database/migration/migrations/v6.1.0.sql Outdated Show resolved Hide resolved
cmd/api/src/api/v2/auth/oidc.go Outdated Show resolved Hide resolved
cmd/api/src/api/v2/auth/oidc.go Outdated Show resolved Hide resolved
cmd/api/src/api/v2/auth/oidc_test.go Outdated Show resolved Hide resolved
cmd/api/src/database/oidc.go Outdated Show resolved Hide resolved
cmd/api/src/database/oidc.go Outdated Show resolved Hide resolved
@mvlipka mvlipka changed the title Bed 4852 OIDC delete endpoint Bed-4852: OIDC delete endpoint Oct 15, 2024
@mvlipka mvlipka changed the title Bed-4852: OIDC delete endpoint Bed-4852: SSO Provider delete endpoint Oct 15, 2024
@mvlipka mvlipka changed the title Bed-4852: SSO Provider delete endpoint Bed-4852: SSO Provider Delete Endpoint Oct 15, 2024
@mvlipka mvlipka added api A pull request containing changes affecting the API code. and removed work in progress This pull request is a work in progress and should not be merged labels Oct 15, 2024
@mvlipka mvlipka added the enhancement New feature or request label Oct 15, 2024
cmd/api/src/database/sso_providers_test.go Outdated Show resolved Hide resolved
cmd/api/src/api/constant.go Outdated Show resolved Hide resolved
@mvlipka mvlipka marked this pull request as ready for review October 16, 2024 17:11
cmd/api/src/database/auth.go Outdated Show resolved Hide resolved
cmd/api/src/database/auth.go Outdated Show resolved Hide resolved
cmd/api/src/database/sso_providers.go Outdated Show resolved Hide resolved
cmd/api/src/database/sso_providers.go Outdated Show resolved Hide resolved
cmd/api/src/model/sso_provider.go Show resolved Hide resolved
cmd/api/src/database/sso_providers.go Show resolved Hide resolved
cmd/api/src/database/sso_providers.go Outdated Show resolved Hide resolved
cmd/api/src/database/auth.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mistahj67 mistahj67 left a comment

Choose a reason for hiding this comment

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

I think there's just a smidge more cleanup and then we 🚀

cmd/api/src/api/v2/auth/auth.go Outdated Show resolved Hide resolved
cmd/api/src/database/oidc_providers.go Outdated Show resolved Hide resolved
cmd/api/src/database/auth.go Show resolved Hide resolved
cmd/api/src/database/sso_providers.go Show resolved Hide resolved
…of letting the PG contraint to remove references
Copy link
Contributor

@mistahj67 mistahj67 left a comment

Choose a reason for hiding this comment

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

Tested locally and hit a few small snags! Otherwise looks solid, love the cleanup ❤️


-- Set the user's saml_provider_id to null when an sso_provider or saml_provider is deleted
ALTER TABLE ONLY users
DROP CONSTRAINT fk_users_saml_provider;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
DROP CONSTRAINT fk_users_saml_provider;
DROP CONSTRAINT IF EXISTS fk_users_saml_provider;

cmd/api/src/api/v2/auth/sso.go Show resolved Hide resolved
cmd/api/src/api/v2/auth/auth.go Show resolved Hide resolved
Copy link
Contributor

@mistahj67 mistahj67 left a comment

Choose a reason for hiding this comment

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

Looks good, feels good, tests good; Send it! 🚀

Comment on lines 152 to 154
} else if err = s.db.DeleteSSOProvider(request.Context(), ssoProviderID); errors.Is(err, database.ErrNotFound) {
api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusNotFound, err.Error(), request), response)
} else if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if err = s.db.DeleteSSOProvider(request.Context(), ssoProviderID); errors.Is(err, database.ErrNotFound) {
api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusNotFound, err.Error(), request), response)
} else if err != nil {
} else if err = s.db.DeleteSSOProvider(request.Context(), ssoProviderID); err != nil {

@mvlipka mvlipka merged commit e7c592e into main Oct 24, 2024
4 checks passed
@mvlipka mvlipka deleted the BED-4852-oidc-delete-endpoint branch October 24, 2024 21:20
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api A pull request containing changes affecting the API code. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants