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 Passkey Support #1099

Merged
merged 19 commits into from
Jan 29, 2025
Merged

Add Passkey Support #1099

merged 19 commits into from
Jan 29, 2025

Conversation

chrisnellis
Copy link
Contributor

@chrisnellis chrisnellis commented Dec 11, 2024

🔧 Changes

📚 References

Attempting to fix #924

🔬 Testing

Ran acceptance tests, observed the recorded JSON API calls and ensured they match the API documentation

📝 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)

@chrisnellis chrisnellis requested a review from a team as a code owner December 11, 2024 02:53
@duedares-rvj
Copy link
Contributor

@chrisnellis Hello!
Thank for so much for this PR.

Here's some steps to get this moving forward:

  1. We shall have to add corresponding expand and flatten rules ref: internal/auth0/connection/expand.go and flatten.go
  2. We don't need to write docs manually. You can run make lint and make generate
  3. Once you're able to test successful creation and modification of connection with this attribute, we can look back at the test cases!

Hope that helps.
Let me know if you have any questions.
Thanks!

@chrisnellis
Copy link
Contributor Author

chrisnellis commented Dec 11, 2024

@duedares-rvj , thanks for your guidance!

I just finished signing my commits after I gave the CONTRIBUTING file a read. I'll start with info from your comment and @ you again when I'm in a better place

@chrisnellis chrisnellis changed the title Add authentication methods to 'options' block Add Passkey Support Dec 13, 2024
@chrisnellis
Copy link
Contributor Author

@duedares-rvj , I think things are in a good place now for this to be reviewed :)

Let me know what you think

@chrisnellis
Copy link
Contributor Author

@kushalshit27, could you and your team please let me know what the review timeline would be for this?

@duedares-rvj
Copy link
Contributor

duedares-rvj commented Jan 14, 2025

@chrisnellis Hello!

I think this slipped down the stack but here's the next steps:

  1. Consider adding authentication_methods into the test cases for Connection module.
  2. Run make lint and make test-acc-record FILTER=name_of_the_test_case

Please tag me and I'll try to respond asap! :)

@chrisnellis
Copy link
Contributor Author

@duedares-rvj
Done :)

@alfechner
Copy link

alfechner commented Jan 23, 2025

Hi @duedares-rvj and @chrisnellis, just wanted to check on this. In my org we're not allowed to deploy any feature without Terraform. So we're desperately waiting for this change 😄

Thanks for the work you put into this!

@chrisnellis
Copy link
Contributor Author

chrisnellis commented Jan 23, 2025

@alfechner, I'm not affiliated with Okta. I'm just a nerd who had some free time to help :)

I'm also waiting on @duedares-rvj for next steps

duedares-rvj
duedares-rvj previously approved these changes Jan 28, 2025
@duedares-rvj duedares-rvj dismissed their stale review January 28, 2025 07:26

Need to add tests

@duedares-rvj
Copy link
Contributor

duedares-rvj commented Jan 28, 2025

@chrisnellis I tried to add some changes but I do not have write access to your repo.

Minor change here:
"passkey": flattenAuthenticationMethodPasskey(authenticationMethods.Passkey)
Use the GetPasskeyI() for Passkey and similarly for Password.

Post that, please run the below:
rm test/data/recordings/TestAccConnection*
make test-acc-record FILTER=TestAccConnection

This would delete old recordings of the tests and re-add the recordings. You'll see the below files deleted and re-added:
test/data/recordings/TestAccConnection.yaml
test/data/recordings/TestAccConnectionAD.yaml
test/data/recordings/TestAccConnectionADFS.yaml
test/data/recordings/TestAccConnectionApple.yaml
test/data/recordings/TestAccConnectionAzureAD.yaml
test/data/recordings/TestAccConnectionClient.yaml
test/data/recordings/TestAccConnectionClients.yaml
test/data/recordings/TestAccConnectionClientsImport.yaml
test/data/recordings/TestAccConnectionClientsPreventErasingEnabledClientsOnCreate.yaml
test/data/recordings/TestAccConnectionConfiguration.yaml
test/data/recordings/TestAccConnectionConfigurationFailsToUpdateWhenEncounteringUnmanagedSecrets.yaml
test/data/recordings/TestAccConnectionCustomSMS.yaml
test/data/recordings/TestAccConnectionEmail.yaml
test/data/recordings/TestAccConnectionFacebook.yaml
test/data/recordings/TestAccConnectionGitHub.yaml
test/data/recordings/TestAccConnectionGoogleApps.yaml
test/data/recordings/TestAccConnectionGoogleOAuth2.yaml
test/data/recordings/TestAccConnectionLinkedin.yaml
test/data/recordings/TestAccConnectionOAuth2.yaml
test/data/recordings/TestAccConnectionOIDC.yaml
test/data/recordings/TestAccConnectionOkta.yaml
test/data/recordings/TestAccConnectionOptionsAttrEmail.yaml
test/data/recordings/TestAccConnectionOptionsAttrInactiveSignUpNegative.yaml
test/data/recordings/TestAccConnectionOptionsAttrNoActiveNegative.yaml
test/data/recordings/TestAccConnectionOptionsAttrPhoneNumber.yaml
test/data/recordings/TestAccConnectionOptionsAttrSetValidationNegative.yaml
test/data/recordings/TestAccConnectionOptionsAttrUserName.yaml
test/data/recordings/TestAccConnectionOptionsAttrUserNameNegative.yaml
test/data/recordings/TestAccConnectionPingFederate.yaml
test/data/recordings/TestAccConnectionSAML.yaml
test/data/recordings/TestAccConnectionSMS.yaml
test/data/recordings/TestAccConnectionSalesforce.yaml
test/data/recordings/TestAccConnectionTwitter.yaml
test/data/recordings/TestAccConnectionWindowslive.yaml

Rest everything looks great in the PR!
Thanks again for the contribution.

@chrisnellis
Copy link
Contributor Author

chrisnellis commented Jan 28, 2025

Hey, @duedares-rvj

I have made the expected change to the flatten.go file.

As far as the acceptance tests go, I need your help setting a set of credentials to run the tests with. In the CONTRIBUTING.md, it calls for a M2M app to be set up. But in the Makefile, it also specifies a particular tenant domain.

If you could either email me valid credentials to use for this, help me run it and record, or direct me to use my own Auth0 tenant for this purpose, let me know and I'll comply.

I also invited you to my repo if you want to make your own changes to my PR

@duedares-rvj duedares-rvj merged commit 0e7b309 into auth0:main Jan 29, 2025
5 of 6 checks passed
@duedares-rvj
Copy link
Contributor

@chrisnellis That's alright.
I think I'll cut a new PR for the recordings. Thanks again! :)

@duedares-rvj duedares-rvj mentioned this pull request Jan 29, 2025
2 tasks
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.

Add Passkey support
3 participants