Skip to content

Conversation

@eliykat
Copy link
Member

@eliykat eliykat commented Feb 22, 2022

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

Further changes required for bitwarden/web#1332.
As discussed here, we now set the SSO configuration defaults in the client-side form, instead of constructing an empty SsoConfigurationData class with defaults and sending it to the client. This PR removes the server-side defaults to avoid confusion about where they're set.

Code changes

  • src/Api/Models/Response/OrganizationSsoResponseModel.cs - leave the Data property null instead of constructing an object with default values.
  • src/Core/Models/Data/SsoConfigurationData.cs - convert all the Url building methods to static methods. This is required now that we may not have an SsoConfigurationData instance in some cases. They actually didn't use any instantiated class properties, so this is more accurate anyway.

Testing requirements

See main ticket

Before you submit

  • I have checked for formatting errors (dotnet tool run dotnet-format --check) (required)
  • If making database changes - I have also updated Entity Framework queries and/or migrations
  • I have added unit tests where it makes sense to do so (encouraged but not required)
  • This change requires a documentation update (notify the documentation team)
  • This change has particular deployment requirements (notify the DevOps team)

Copy link
Contributor

@cscharf cscharf 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.

@eliykat eliykat merged commit 3443fe9 into master Mar 3, 2022
@eliykat eliykat deleted the fix/sso-validation-v2 branch March 3, 2022 21:09
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.

3 participants