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.

Sign up for GitHub

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

Jump to bottom

[server] move FGA calls into AuthProviderService #19017

Merged
merged 4 commits into from
Nov 10, 2023
Merged

[server] move FGA calls into AuthProviderService #19017

merged 4 commits into from
Nov 10, 2023

Conversation

AlexTugarev
Copy link
Member

@AlexTugarev AlexTugarev commented Nov 6, 2023 •
edited
Loading

Description

This change set was extracted out of #19008.

What happens here is a refactoring of the json-rpc api and implementation in order to make it, i.e. the AuthProviderService, reusable to implement the public-api interfaces.

Summary generated by Copilot

🤖 Generated by Copilot at 1010fde

This pull request refactors the auth provider API and the GitpodServer interface to improve the security, clarity, and modularity of the auth provider operations. It introduces new methods for managing auth providers at the user and org level, and applies rate limiting and authorization checks to them. It also updates the UserDeletionService class to use the new method for deleting the user's own auth providers.

Related Issue(s)

Fixes #

How to test

  • verify managing scopes works as usual, i.e. you can update your git token in user settings
  • verify creating of new git integrations works as usual, on user-level as well as on Org-level
  • verify no credentials are sent over the websocket channel (as we're touching mapping of shapes in this PR)

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 mentioned this pull request Nov 6, 2023
Merged
14 tasks
@roboquat roboquat added size/XL and removed size/L labels Nov 6, 2023
@AlexTugarev
Copy link
Member Author

AlexTugarev commented Nov 6, 2023 •
edited
Loading

DI issue to be solved:

Circular dependency found: 
Server --> Authenticator --> Symbol(HostContextProvider) --> AuthProviderService --> Symbol(HostContextProvider)

✅

@AlexTugarev AlexTugarev force-pushed the at/server-authprovider-service branch from 3d48608 to 931fbcb Compare November 7, 2023 13:34
@roboquat roboquat added size/XXL and removed size/XL labels Nov 7, 2023
@akosyakov
Copy link
Member

@AlexTugarev is good to review? asking because it is still in draft

@AlexTugarev AlexTugarev force-pushed the at/server-authprovider-service branch from 53e3ecb to 42b51f3 Compare November 8, 2023 11:09
@AlexTugarev AlexTugarev force-pushed the at/server-authprovider-service branch from 37433cc to ab06e0b Compare November 8, 2023 17:22
@AlexTugarev AlexTugarev marked this pull request as ready for review November 8, 2023 17:24
@AlexTugarev AlexTugarev requested a review from a team as a code owner November 8, 2023 17:24
@AlexTugarev AlexTugarev force-pushed the at/server-authprovider-service branch from ab06e0b to a5ac3da Compare November 8, 2023 18:47
@AlexTugarev
[server] move FGA calls into AuthProviderService
341a5bd 
* split internal upsert method `updateAuthProvider` into create and update
* refactor: move `getAuthProviders` logic from gitpod-server-impl to auth-provider-service
* adding db tests for auth provider server
* use redacted results in service
@AlexTugarev AlexTugarev force-pushed the at/server-authprovider-service branch from a5ac3da to 341a5bd Compare November 8, 2023 18:47
@akosyakov
Copy link
Member

@mustard-mh it would be good if you review code and test as well

@mustard-mh mustard-mh self-requested a review November 9, 2023 05:57
mustard-mh
mustard-mh approved these changes Nov 9, 2023
View reviewed changes
Copy link
Contributor

@mustard-mh mustard-mh left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested with BBS 🎉

✅ It can create/delete org SCM, and able to connect and create ws
SCR-20231109-ohex

✅ After delete org SCM, connected one in user setting deleted too

✅ User level SCM create/delete/connect and workspace create also works
SCR-20231109-omai

@AlexTugarev
Copy link
Member Author

We've been testing with Anton and one situation was unclear which deserves to be covered in a test case. getAuthProviders should return providers owned by Orgs where the subject is a regular member of. There is a test case for owners, but not for members. Also this is to be checked with Dogfood cell to provide a consistent behavior.

@AlexTugarev
add more tests
b54761f 
* as regular member, should find org-level providers if no built-in providers present
* as regular member, should find only built-in providers if present
@AlexTugarev
Copy link
Member Author

@akosyakov, the behavior we've seen during testing is consistent with previous decisions. I've added test cases to cover the very in b54761f

@akosyakov
Copy link
Member

@AlexTugarev good, do you need any other support or good to land?

@AlexTugarev
Copy link
Member Author

/unhold

@roboquat roboquat merged commit b2d5018 into main Nov 10, 2023
15 checks passed
@roboquat roboquat deleted the at/server-authprovider-service branch November 10, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akosyakov akosyakov akosyakov left review comments

@mustard-mh mustard-mh mustard-mh approved these changes

Assignees
No one assigned
Labels
size/XXL team: team-experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlexTugarev @akosyakov @mustard-mh @roboquat