Add mirror access control #1085

ikhoon · 2025-01-03T08:23:30Z

Motivation:

Sometimes you need to restrict access to specific repositories without allowing mirroring for all repositories. These repositories may contain sensitive information or code that is only available to a select group of members.

Modifications:

Add MirrorAccessController that can allow or disallow access to the remote repositories for mirroring.
- It is used to check if a scheduled mirroring task or a task triggered by UI is allowed to access.
- MirrorListener.onCreate() and MirrorListener.onUpdate() events are newly added. The events could be used as an extension point to detect a new remote URL pattern and integrate external systems such as a workflow or an email notification.
Add CRUD REST API for administrators
- GET /api/v1/mirror/access to retrieve all mirror access controls.
- POST /api/v1/mirror/access to create a mirror access controls.
- and so on...
In addition to the REST API, a mirror access control can be added with MirrorAccessController.allow(...) API which is exposed by PluginContext.mirrorAccessController().
Add CrudRepository abstraction to easily create and update a collection of entities in a directory.
- This API would be useful especially when we implement UI-based CRUD operations.
Add mirror access control UI for administrators
- CRUD operations are supported.

Result:

Administrators can restrict access to remote repositories when mirroring.
You can receive notifications when a new mirror is created or an existing mirror is updated through MirrorListener.

Motivation: Sometimes you need to restrict access to specific repositories without allowing mirroring for all repositories. These repositories may contain sensitive information or code that is only available to a select group of members. Modifications: - Add `MirrorAccessController` that can allow or disallow access to the remote repositories for mirroring. - It is used to check a scheduled mirroring task or a task triggered by UI is allowed to access. - `MirrorListener.onCreate()` and `MirrorListener.onUpdate()` events are newly added. The events could be used as an extension point to detect a new remote URL pattern and integrate external systems such as a workflow or a email notification. - Add CRUD REST API for administrators - GET `/api/v1/mirror/access` to retrieve all mirror access controls. - POST `/api/v1/mirror/access` to create a mirror access controls. - and so on... - In addition to the REST API, a mirror access control can be added with `MirrorAccessController.allow(...)` API which is exposed by `PluginContext.mirrorAccessController()`. - Add `CrudRepository` abstraction to easily create and update a collection of entities in a directory. - This API would be useful especially when we implement UI-based CRUD operations. - Add mirror access control UI for administrators - CRUD are supported. Result: - Administrators can restrict access to remote repositories when mirroring. - You can receive notifications when a new mirror is created or an existing mirror is updated through `MirrorListener`.

ikhoon · 2025-01-03T08:24:32Z

minwoox

Basically looks great! Will also review the UI part. 👍

it/mirror/src/test/java/com/linecorp/centraldogma/it/mirror/git/MirrorAccessControlTest.java

it/mirror/src/test/java/com/linecorp/centraldogma/it/mirror/git/MirrorRunnerTest.java

.../src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/HasRevision.java

it/mirror/src/test/java/com/linecorp/centraldogma/it/mirror/git/ZoneAwareMirrorTest.java

server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java

...ain/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMirrorAccessController.java

minwoox · 2025-01-07T01:00:35Z

...ain/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMirrorAccessController.java

+    }
+
+    @Override
+    public CompletableFuture<Boolean> allow(String targetPattern, String reason, int order) {


Is there any reason that you encourage using this via PluginContext.mirrorAccessController() instead of the REST API?

It is possible to use REST API in a Plugin but it can be a hassle.
The plugin needs to manage an access token for each phase.

I was thinking of implementing a plugin to expose a callback as a REST API.
When the callback is invoked, a target pattern will be added to a whitelist or a blacklist via these APIs.

It is possible to use REST API in a Plugin but it can be a hassle.

What I meant was exposing this API via MirrorAccessControlService. Is there any reason for this?

MirrorAccessController is going to be used in a plugin to pragmatically add an access control via the API.

minwoox · 2025-01-07T01:02:49Z

...ain/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMirrorAccessController.java

+        logger.info("Allowing the target pattern: {}", accessControl);
+        // If there is a duplicate target pattern, the order will be considered first.
+        // If the order is the same, the latest one will be considered first.
+        return repository().save(accessControl, author).thenApply(unused -> true);


How can we delete this access control later? Do we just need to disallow it?

Currently, it isn't supported by MirrorAccessControler API. If necessary, should we use the UI to manage it?

Considering the current usability, I thought the design is sufficient.

I see. I was just curious because the ID is automatically generated.

jrhee17 · 2025-01-07T02:42:22Z

...ain/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMirrorAccessController.java

+            if (acl.isEmpty()) {
+                // If there is no access control, it is allowed by default.
+                return Streams.stream(repoUris)
+                              .collect(toImmutableMap(uri -> uri, uri -> true));


Question) Is uri guaranteed to be unique?

Good point. uri is not unique.

jrhee17 · 2025-01-07T02:46:54Z

.../src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingService.java

+                                  if (!allowed) {
+                                      logger.debug("The mirroring from {} is not allowed. mirror: {}",
+                                                   m.remoteRepoUri(), m);
+                                      continue;
+                                  }
+                              } catch (Exception e) {
+                                  logger.warn("Failed to check the access control. mirror: {}",
+                                              m, e);
+                                  continue;
+                              }


Question) Would it be more consistent behavior to notify MirrorListener and log from there if needed?

MirrorListener.onDisallowed() is added to notify if a mirror is disallowed.
I didn't address the warning log for the exception because it will be raised rarely and the error may be a system-level error.

jrhee17 · 2025-01-07T03:08:02Z

server/src/main/java/com/linecorp/centraldogma/server/mirror/MirrorListener.java

+    /**
+     * Invoked when a new {@link Mirror} is created.
+     */
+    void onCreate(Mirror mirror, MirrorAccessController accessController);


Question) What do you think of only passing the mirrorId here and instead creating a MirrorContext similar to how PluginContext is defined which contains ProjectManager, CommandExecutor, etc..

CentralDogma could maintain an internal CompositeMirrorListener which holds this context, and pass it on to user-defined child MirrorListeners.

This way

we can actually handle the failure from the extra lookup in when creating/updating a mirror

there is more freedom in MirrorListener to modify/read from the repository

I agree that the extra lookup is useless for the default implementation but the overhead would not be significant.

However, it is not enough to just give a mirror ID. Since a snapshot of when it was created is needed, a revision should also be provided. Instead of adding more contexts to the API, I think the current style is not bad.

It may be possible to give more freedom, but I doubt the features are useful at this point.

we can actually handle the failure from the extra lookup in when creating/updating a mirror

For more context, I imagined that we will notify users when a mirror is created/updated, but not allowed.
My concern wasn't really about overhead, but rather if there is a way to monitor/apply fallback logic if the callback isn't invoked due to failure from the extra IO.
(I doubt we will be looking at exception logs since we aren't actively monitoring logs at the moment)

I'm fine with the current design, but wanted to make the above point clear

I agree that the async processing is not good from a transaction perspective. If we only pass the mirror ID, there is an advantage that MirrorListener is always called when the commit is pushed.

Initially, I wished to design a simple API. I will consider improving this point in the future.

codecov · 2025-01-07T07:59:40Z

Codecov Report

Attention: Patch coverage is 63.15789% with 175 lines in your changes missing coverage. Please review.

Project coverage is 70.19%. Comparing base (f4dda40) to head (63d5399).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...rp/centraldogma/internal/api/v1/MirrorRequest.java	44.77%	37 Missing ⚠️
...internal/mirror/DefaultMirrorAccessController.java	54.16%	31 Missing and 2 partials ⚠️
...ernal/api/sysadmin/MirrorAccessControlRequest.java	45.16%	17 Missing ⚠️
...ldogma/server/internal/api/MirroringServiceV1.java	56.25%	13 Missing and 1 partial ⚠️
...erver/internal/mirror/CompositeMirrorListener.java	47.61%	11 Missing ⚠️
...rnal/storage/repository/git/GitCrudRepository.java	75.55%	10 Missing and 1 partial ⚠️
...nternal/storage/repository/DefaultHasRevision.java	33.33%	10 Missing ⚠️
...ma/server/internal/mirror/MirrorAccessControl.java	72.72%	9 Missing ⚠️
...erver/internal/mirror/MirrorSchedulingService.java	50.00%	5 Missing and 1 partial ⚠️
...traldogma/server/internal/mirror/MirrorRunner.java	79.16%	4 Missing and 1 partial ⚠️
... and 8 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1085      +/-   ##
============================================
- Coverage     70.29%   70.19%   -0.11%     
- Complexity     4284     4372      +88     
============================================
  Files           427      441      +14     
  Lines         17258    17622     +364     
  Branches       1915     1939      +24     
============================================
+ Hits          12131    12369     +238     
- Misses         4077     4207     +130     
+ Partials       1050     1046       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

minwoox · 2025-01-07T11:18:02Z

Sorry for causing the conflict. 😓

ikhoon · 2025-01-07T11:54:58Z

No worries. I expected it in advance.

minwoox

Looks great! 👍 👍 👍

webapp/src/dogma/common/components/DeleteConfirmationModal.tsx

webapp/src/dogma/features/project/settings/tokens/DeleteToken.tsx

webapp/src/dogma/features/settings/SettingView.tsx

webapp/src/dogma/features/settings/mirror-access/MirrorAccessControlForm.tsx

ikhoon added the new feature label Jan 3, 2025

ikhoon added this to the 0.74.0 milestone Jan 3, 2025

ikhoon requested review from jrhee17, minwoox and trustin as code owners January 3, 2025 08:23

fix tests

8b2dcfc

minwoox reviewed Jan 7, 2025

View reviewed changes

jrhee17 approved these changes Jan 7, 2025

View reviewed changes

ikhoon added 2 commits January 7, 2025 16:27

address comments

f5d2ff5

Merge branch 'main' into mirroring-acl

63d5399

minwoox mentioned this pull request Jan 7, 2025

Move Mirror Config to Each Repository #1086

Open

Merge branch 'main' into mirroring-acl

2058e61

minwoox approved these changes Jan 8, 2025

View reviewed changes

ikhoon added 3 commits January 8, 2025 14:52

remove cruft

23601d2

delete the empty file

21f7b04

remove contstructor in DefaultMirrorAccessController

e8ece8f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mirror access control #1085

Add mirror access control #1085

ikhoon commented Jan 3, 2025

ikhoon commented Jan 3, 2025

minwoox left a comment

minwoox Jan 7, 2025

ikhoon Jan 7, 2025

minwoox Jan 7, 2025

ikhoon Jan 8, 2025

minwoox Jan 7, 2025

ikhoon Jan 7, 2025

minwoox Jan 7, 2025

jrhee17 Jan 7, 2025

ikhoon Jan 7, 2025

jrhee17 Jan 7, 2025

ikhoon Jan 7, 2025

jrhee17 Jan 7, 2025

ikhoon Jan 7, 2025

jrhee17 Jan 8, 2025

ikhoon Jan 8, 2025

codecov bot commented Jan 7, 2025

minwoox commented Jan 7, 2025

ikhoon commented Jan 7, 2025

minwoox left a comment

Add mirror access control #1085

Are you sure you want to change the base?

Add mirror access control #1085

Conversation

ikhoon commented Jan 3, 2025

ikhoon commented Jan 3, 2025

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 7, 2025

Codecov Report

minwoox commented Jan 7, 2025

ikhoon commented Jan 7, 2025

minwoox left a comment

Choose a reason for hiding this comment