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

fix: Remove Service Account from Zitadel when API key is deleted. #3908

Merged
merged 66 commits into from
Sep 16, 2024

Conversation

bryan-robitaille
Copy link
Contributor

@bryan-robitaille bryan-robitaille commented Jun 26, 2024

Summary | Résumé

This PR adds the functionality to:

  • Create or use an existing Service Account for a template. It would use the same service account if ever there is a break in logic and the account was not deleted properly. It saves the user flow by not throwing an error and requiring support. A new key is generated and replaces the old key so the security risk and impact is minimal
  • Deletes a Service Account when a key is deleted.
  • Adds a commented out section where Zitadel can be integrated with Next-Auth. While I'm usually not a fan of leaving commented code in a PR this configuration was not easy to implement and required a lot of testing and research to get the settings correct.
  • Deletes a Service Account when a form is deleted

Other file changes that were part of small refactoring while going through the code base:

  • Update yarn to 4.4.1 from 4.1.1
  • CHANGELOG.md was auto-formatted on save due to mismatching asterix vs dash usage in the file.
  • rehydrateFormResponse was missing a indentation tab
  • use_client.sh was a legacy script that was only required during the migration of Pages Router to App Router
  • Remove the translations for formTimer and reCatpcha flags which no longer exist.
  • redisConnector initialization had a duplicate step to set the module scope property
  • In Jest globally mock Redis implementation with ioredis-mock
  • In Jest globally mock Prisma only when using Node env
  • In Jest set global env to Node and add direct changes to jsdom in required files
  • responsibilities.md has an additional whitespace at the end of sentence.
  • next_auth types: removed the defined User type which was not used in the app

Test instructions | Instructions pour tester la modification

*Must have an account on Zitadel in Staging

  1. Login to the Web App with the Zitadel flag "ON"
  2. Create or go to an existing template.
  3. Create an API key (user with template ID should be created in Zitadel)
  4. Refresh an API Key (user not changed and key deleted and recreated in Zitadel)
  5. Delete an API Key (user remove from Zitadel)

Unresolved questions / Out of scope | Questions non résolues ou hors sujet

User Errors back to the interface are currently out of scope while we wait for designs on how the API Settings should be created.

Pull Request Checklist

Please complete the following items in the checklist before you request a review:

  • Have you completely tested the functionality of change introduced in this PR? Is the PR solving the problem it's meant to solve within the scope of the related issue?
  • The PR does not introduce any new issues such as failed tests, console warnings or new bugs.
  • If this PR adds a package have you ensured its licensed correctly and does not add additional security issues?
  • Is the code clean, readable and maintainable? Is it easy to understand and comprehend.
  • Does your code have adequate comprehensible comments? Do new functions have docstrings?
  • Have you modified the change log and updated any relevant documentation?
  • Is there adequate test coverage? Both unit tests and end-to-end tests where applicable?
  • If your PR is touching any UI is it accessible? Have you tested it with a screen reader? Have you tested it with automated testing tools such as axe?

@bryan-robitaille bryan-robitaille changed the title feature: Zitadel Integration with Service Account WIP - feature: Zitadel Integration with Service Account Jun 26, 2024
@bryan-robitaille bryan-robitaille marked this pull request as ready for review August 27, 2024 14:12
Copy link
Contributor

@craigzour craigzour left a comment

Choose a reason for hiding this comment

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

Will now test it before approving but I can already share a few comments I added to your PR

CHANGELOG.md Show resolved Hide resolved
lib/templates.ts Outdated Show resolved Hide resolved
lib/serviceAccount.ts Outdated Show resolved Hide resolved
@craigzour
Copy link
Contributor

craigzour commented Aug 28, 2024

@bryan-robitaille

I tested your PR locally and it does work well (including the delete template scenario with the right flag).

I know you mentioned that client errors are not yet implemented but I still wanted to share a scenario I tested, just out of curiosity, and that revealed a possible problem for the future. Let's say you have a form with an active API service account linked associated to it. You decide to manually delete that account through the Zitadel console then you go back to the web application and refresh the page. You will see that the API key does not exist anymore but if you try to create a new one you will get a database conflict because the previous one is still in there.

@bryan-robitaille bryan-robitaille enabled auto-merge (squash) September 16, 2024 17:12
@craigzour craigzour self-requested a review September 16, 2024 17:13
@bryan-robitaille bryan-robitaille merged commit 13e6671 into develop Sep 16, 2024
13 checks passed
@bryan-robitaille bryan-robitaille deleted the feature/service_account branch September 16, 2024 17:18
@patheard patheard mentioned this pull request Sep 18, 2024
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.

2 participants