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

Add addons.samlp.issuer field to client resource #334

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

xens
Copy link
Contributor

@xens xens commented Oct 1, 2022

🔧 Changes

This PR adds support for the issuer field within the samlp client addon. The issuer defaults to urn:your_tenant.auth0.com if not set explicitly. It can be set to something else including the proto part, for example; https://mydomain.com.

📚 References

🔬 Testing

I've added the option to the existing testAccClientConfig test.

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

@xens xens requested a review from a team as a code owner October 1, 2022 08:42
@xens xens force-pushed the feat/add_samlp_audience branch 2 times, most recently from 91048da to 4afd3d7 Compare October 2, 2022 04:21
@xens xens changed the title DRAFT: feat: client: add samlp issuer feat: client: add samlp issuer option Oct 2, 2022
@zonArt
Copy link

zonArt commented Oct 3, 2022

Thanks for the PR, it's working like a charm

@xens xens force-pushed the feat/add_samlp_audience branch 2 times, most recently from 0376a67 to 7e1a452 Compare October 4, 2022 01:05
@xens
Copy link
Contributor Author

xens commented Oct 6, 2022

One test is failing and I'm not 100% sure why. The Terraform plan is failing because Terraform wants to change things which is normal in this case, is that something that must be manually applied by maintainers when a resource changes?

Also I'm a bit confused by the recordings folder and how it works exactly, I've added the expected config and output inside https://github.com/auth0/terraform-provider-auth0/pull/334/files#diff-c580a6ecc09747c0539023d5ab939abf42d4f33359936761380ae34eba9a24e6R84. Do I also need to edit the recordings folder or is it automagically generated ?

@sergiught
Copy link
Contributor

Hey @xens 👋🏻 first of all thanks for the contribution! 🌟

So the tests make use of pre-recorded http interactions found in the recordings folder. To add new recordings or to simply recreate new ones when functionality is added you can:

The current test fails with:

resource_auth0_client_test.go:60: Step 1/2 error: Check failed: Check 19/24 error: auth0_client.my_client: Attribute 'addons.0.samlp.0.issuer' expected "https://example.com/", got ""

Because we haven't updated all the registered interactions for that test to include the issuer field.

Hope this helps, but if you still experience issues we can help out and adjust the recordings together after we finish tackling #14.

@xens xens force-pushed the feat/add_samlp_audience branch from 7e1a452 to 19f2973 Compare October 8, 2022 08:38
@xens
Copy link
Contributor Author

xens commented Oct 8, 2022

thanks @sergiught for the detailed explanations! I've been able to run the tests locally and fix the required recordings. However the tests are still failing on the Terraform plan.

@sergiught
Copy link
Contributor

Hey @xens 👋🏻 thanks for that! 👍🏻

I'll be able to help get this in the right place tomorrow, no worries about the conflicts either. If I can push to your branch I'll fix them as well.

@sergiught sergiught marked this pull request as draft October 12, 2022 13:36
@sergiught
Copy link
Contributor

Hey @xens 👋🏻 , I set this branch to draft mode while we get this in the right place.

@sergiught sergiught force-pushed the feat/add_samlp_audience branch from 19f2973 to e2e500a Compare October 12, 2022 15:30
@sergiught sergiught changed the title feat: client: add samlp issuer option Add addons.samlp.issuer field to client resource Oct 12, 2022
@sergiught sergiught marked this pull request as ready for review October 12, 2022 15:36
Copy link
Contributor

@sergiught sergiught left a comment

Choose a reason for hiding this comment

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

Thanks a lot @xens for your contribution! Everything was looking great, I just helped out with rebasing and fixing conflicts. Please give it another look as well and let me know when you're done.

@codecov-commenter
Copy link

Codecov Report

Base: 86.80% // Head: 86.82% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (e2e500a) compared to base (ca8a3e0).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #334      +/-   ##
==========================================
+ Coverage   86.80%   86.82%   +0.01%     
==========================================
  Files          40       40              
  Lines        8476     8485       +9     
==========================================
+ Hits         7358     7367       +9     
  Misses        858      858              
  Partials      260      260              
Impacted Files Coverage Δ
internal/provider/resource_auth0_client.go 97.14% <100.00%> (+0.01%) ⬆️
internal/provider/structure_auth0_client.go 81.43% <100.00%> (+0.17%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@xens
Copy link
Contributor Author

xens commented Oct 12, 2022

@sergiught thanks for the review, lgtm! 🥳

@sergiught sergiught merged commit 4be47cb into auth0:main Oct 12, 2022
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.

4 participants