Added ProviderType attribute in saml settings#1303
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for the provider-type SAML admin setting by exposing it in the Go client models and covering it with integration tests.
Changes:
- Add
ProviderTypetoAdminSAMLSetting. - Add
ProviderTypetoAdminSAMLSettingsUpdateOptionsso the setting can be updated via the API. - Extend SAML admin integration tests to read and update
provider-type.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
CHANGELOG.md |
Documents the new ProviderType field for SAML admin settings. |
admin_setting_saml.go |
Adds JSONAPI attribute mapping for provider-type on read and update option structs. |
admin_setting_saml_integration_test.go |
Adds assertions and a new subtest to exercise ProviderType behavior end-to-end. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Maed223
left a comment
Maed223
left a comment
There was a problem hiding this comment.
Smoke tested and works as expected. Just an open question below.
admin_setting_saml.go
Outdated
| WantAssertionsSigned *bool `jsonapi:"attr,want-assertions-signed,omitempty"` | ||
| SignatureSigningMethod *string `jsonapi:"attr,signature-signing-method,omitempty"` | ||
| SignatureDigestMethod *string `jsonapi:"attr,signature-digest-method,omitempty"` | ||
| ProviderType *string `jsonapi:"attr,provider-type,omitempty"` |
There was a problem hiding this comment.
I think having a type with a set of consts would be a helpful user hint here
Such as:
type ProviderType string
const (
ProviderTypeOkta ProviderType = "okta"
...
)There was a problem hiding this comment.
Though if the value here isn't being constrained in any way, then ignore this point.
There was a problem hiding this comment.
thx for the suggestion, I have added the type.
| }) | ||
|
|
||
| t.Run("with provider type defined", func(t *testing.T) { | ||
| providerTypesForTesting := []string{"okta", "entra", "saml", "unknown", "error"} |
There was a problem hiding this comment.
The inclusion of the assertion on "unknown" here feels a bit off, though I'm not very familiar with the backing API. If the intention is to have a constrained set, then the way the test is currently setup could hide regressions. Open question for me is whether the provider type is intentionally open ended to allow non-standard values.
Nit: this test could be better expressed being table driven. Just so the expected intentions/outcomes are easily parseable.
There was a problem hiding this comment.
backend can only have these 4 values: "okta", "entra", "saml" and "unknown".
"unknown" is for backward compatibility, so it will be the default value.
I have update the test to table driven, thx for the suggestion.
and I have also added validtion as well.
…provider type tests to use table-driven format
|
Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes. |
Description
This PR introduces support for configuring the
provider-typeSAML setting. It allows administrators to specify the identity provider type (e.g.,okta,entra, or genericsaml) directly in their TFE admin SAML configuration.Testing plan
ProviderTypefield insideTestAdminSettings_SAML_Readto confirm the value is successfully read.TestAdminSettings_SAML_Updateto verify updates pass for valid provider types (okta,entra,saml,unknown) and appropriately error out on invalid ones.External links
Output from tests
Rollback Plan
we can revert this pr
Changes to Security Controls
NA