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

[public-api] Add AuthProviderService service #19008

Merged
merged 9 commits into from
Nov 13, 2023
Merged

Conversation

AlexTugarev
Copy link
Member

@AlexTugarev AlexTugarev commented Nov 3, 2023

Description

Depends on #19041, which depends on #19017

This PR adds and implements public-api service for AuthProviders

Summary generated by Copilot

🤖 Generated by Copilot at 55bd7e7

This pull request adds the AuthProviderService to the experimental API, which is a service for managing authentication providers. It generates the code for the service using different protocols and plugins, such as gRPC, Connect, and protoc-gen-go-grpc. It also adds the proto file that defines the service and its messages and enums.

Related Issue(s)

Fixes #

How to test

Documentation

Preview status

Gitpod was successfully deployed to your preview environment.

Build Options

Build
  • /werft with-werft
    Run the build with werft instead of GHA
  • leeway-no-cache
  • /werft no-test
    Run Leeway with --dont-test
Publish
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer
  • analytics=segment
  • with-dedicated-emulation
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh. If enabled, with-preview and with-large-vm will be enabled.
  • with-monitoring

/hold

@AlexTugarev AlexTugarev force-pushed the at/authprovider_api branch 10 times, most recently from 8441e9e to f222e04 Compare November 6, 2023 15:55
@AlexTugarev AlexTugarev changed the base branch from main to at/server-authprovider-service November 6, 2023 15:56
@AlexTugarev AlexTugarev changed the title WIP AuthProviderService API [public-api] Add AuthProviderService service Nov 6, 2023
@AlexTugarev AlexTugarev force-pushed the at/authprovider_api branch 8 times, most recently from f8f6da6 to a63c62c Compare November 7, 2023 13:33
@AlexTugarev AlexTugarev marked this pull request as ready for review November 10, 2023 11:31
@AlexTugarev AlexTugarev requested a review from a team as a code owner November 10, 2023 11:31
}
const clientId = request.clientId;
const clientSecret = request.clientSecret;
if (!clientId || typeof clientSecret === "undefined") {
Copy link
Member

Choose a reason for hiding this comment

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

we don't require both? it is rather at least one of them should be present

Copy link
Member

Choose a reason for hiding this comment

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

I used following pattern here:

const update: Partial<ProtocolOrganizationSettings> = {};
if (typeof req.workspaceSharingDisabled === "boolean") {
update.workspaceSharingDisabled = req.workspaceSharingDisabled;
}
const hasDefaultWorkspaceImage = typeof req.defaultWorkspaceImage === "string";
const resetDefaultWorkspaceImage = req.resetMask?.paths.includes("defaultWorkspaceImage");
if (hasDefaultWorkspaceImage && resetDefaultWorkspaceImage) {
throw new ConnectError(
"defaultWorkspaceImage cannot be set and reset at the same time",
Code.InvalidArgument,
);
}
if (resetDefaultWorkspaceImage) {
update.defaultWorkspaceImage = null;
} else if (hasDefaultWorkspaceImage) {
update.defaultWorkspaceImage = req.defaultWorkspaceImage;
}
if (Object.keys(update).length === 0) {
throw new ConnectError("nothing to update", Code.InvalidArgument);
}

not sure whether we want to align

Copy link
Member Author

Choose a reason for hiding this comment

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

This would mean, we need to change the Dashboard part and retest the behavior. I'd rather avoid.

Copy link
Member

@akosyakov akosyakov Nov 10, 2023

Choose a reason for hiding this comment

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

  • We don't build API for one client (dashboard).
  • If dashboard already provides both values it should not be an issue anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, now I got you.

we don't require both? it is rather at least one of them should be present

wasn't clear from that.

So, even if Dashboard is always sending the tuple, we may allow for adjusting a single attribute here as well, right? We can change that. Just the focus wasn't on changing existing behavior for the migration phase.

Copy link
Member Author

Choose a reason for hiding this comment

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

resolved in fc3be82

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

pre approved, but please to align on contract, and unnecessary checks before mergin 🙏

@AlexTugarev AlexTugarev marked this pull request as ready for review November 10, 2023 14:48
allow to update clientId or clientSecret separately.
@AlexTugarev
Copy link
Member Author

@akosyakov, I've address the comments and added tests.

Going to test in preview and proceed afterwards.

@AlexTugarev
Copy link
Member Author

/unhold

@roboquat roboquat merged commit 0e00e3d into main Nov 13, 2023
25 of 26 checks passed
@roboquat roboquat deleted the at/authprovider_api branch November 13, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants