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

feat: Enable Zitadel Integration for API Authentication #4147

Merged
merged 8 commits into from
Aug 9, 2024

Conversation

wmoussa-gc
Copy link
Contributor

@wmoussa-gc wmoussa-gc commented Aug 1, 2024

Summary | Résumé

Most of the work was ported from Bryan's original PR #3908. The only change made was in the management of Zitadel's admin key.

Flow

Screen.Recording.2024-08-06.at.8.52.55.PM.mov

Machine Service Accounts

1 Form = 1 Account
image

Test instructions | Instructions pour tester la modification

  1. Switch on the feature flag for Zitadel
  2. Go to Settings > API Access
  3. Click Generate Key

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

TBD

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?

@wmoussa-gc wmoussa-gc marked this pull request as draft August 1, 2024 11:38
@wmoussa-gc wmoussa-gc marked this pull request as ready for review August 7, 2024 02:51
let zitadelClient: ManagementServiceClient | null = null;

const getZitadelSettings = async () => {
if (!process.env.ZITADEL_PROVIDER) throw new Error("No value set for Zitadel Provider");
Copy link
Contributor Author

@wmoussa-gc wmoussa-gc Aug 7, 2024

Choose a reason for hiding this comment

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

You can find this value and the one below in 1Password. These need to be injected into the Terraform code. Bryan used the Forms settings to store the values encrypted. Not sure what was the reason.

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.

Looks good to me! It works as expected ;) Just want to flag that we still need to handle errors across the whole feature. We could create a ticket for it unless you want to tackle it now. What do you think?

};

const _refreshKey = async (templateId: string) => {
const key = await refreshKey(templateId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan on adding error handling in a future iteration? I noticed the lib code can throw various type of errors at the moment.

<CircleCheckIcon className="mr-2 inline-block w-9 fill-green-700" />
{t("settings.api.keyExists")}
</div>
<Button theme="primary" className="mr-4" onClick={() => deleteKey(id)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

The deleteKey function is async and can also throw errors. Not a big deal but it would be good to let the user know whether the key was successfully deleted or not.

@wmoussa-gc
Copy link
Contributor Author

We could create a ticket for it unless you want to tackle it now

I'd like to see a happy path working in staging, and then we can focus on stabilizing the code. What do you think?

@craigzour
Copy link
Contributor

We could create a ticket for it unless you want to tackle it now

I'd like to see a happy path working in staging, and then we can focus on stabilizing the code. What do you think?

Sounds good to me! As long as we do not forget about it :)

@craigzour craigzour self-requested a review August 8, 2024 00:08
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.

LGTM!

@wmoussa-gc wmoussa-gc merged commit 200ab05 into develop Aug 9, 2024
13 checks passed
@wmoussa-gc wmoussa-gc deleted the feature/service_account2 branch August 9, 2024 13:22
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