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

Reject invalid mirroring URI #1012

Merged
merged 3 commits into from
Aug 16, 2024
Merged

Reject invalid mirroring URI #1012

merged 3 commits into from
Aug 16, 2024

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Aug 14, 2024

Motivation:

An expected remote URI for mirroring is <domain.com>/<org>/<repo>.git. However, both the mirror UI and REST API didn't validate the input.

For example, a SSH URL, github.com:line/centraldogma.git is not valid in Central Dogma but can be committed. Exceptions are raised later when it is used.

Additionally, I also fixed a bug where settings tabs are not visible to admin that I found while fixing this bug.

Modifications:

  • Improved the regular expression for remoteUri to strictly check the input.
  • Validate a remote URI in the mirror REST API.
  • Always set the project role of adminstrator to OWNER

Result:

Invalid remote URIs are now rejected by the mirror UI and API.

Motivation:

An expected remote URI for mirroring is `<domain.com>/<org>/<repo>.git`.
However, both the mirror UI and REST API didn't validate the input.

For example, a SSH URL, `github.com:line/centraldogma.git` is not valid
in Central Dogma but can be committed. Exceptions are raised later when
it is used.

Additionally, I also fixed a bug where settings tabs are not visible to
admin that I found while fixing this bug.

Modifications:

- Improved the regular expression for `remoteUri` to strictly check the
  input.
- Validate a remote URI in the mirror REST API.
- Always set the project role of adminstrator to `OWNER`

Result:

Invalid remote URIs are now rejected by the mirror UI and API.
@ikhoon ikhoon added the defect label Aug 14, 2024
@ikhoon ikhoon added this to the 0.68.1 milestone Aug 14, 2024
@@ -99,6 +100,11 @@ public MirrorConfig(@JsonProperty("id") String id,
this.localRepo = requireNonNull(localRepo, "localRepo");
this.localPath = firstNonNull(localPath, "/");
this.remoteUri = requireNonNull(remoteUri, "remoteUri");

// Validate the remote URI.
final String suffix = remoteUri.getScheme().equals(SCHEME_DOGMA) ? "dogma" : "git";
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we do this in the MirrorDto or MirroringServiceV1?
How about also checking if the local repo exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we do this in the MirrorDto or MirroringServiceV1?

I chose this place because the segments of remote URI in MirrorDto need to be assembled.

private static MirrorConfig converterToMirrorConfig(MirrorDto mirrorDto) {
final String remoteUri =
mirrorDto.remoteScheme() + "://" + mirrorDto.remoteUrl() +
MirrorUtil.normalizePath(mirrorDto.remotePath()) + '#' + mirrorDto.remoteBranch();
return new MirrorConfig(

How about also checking if the local repo exists?

That would be covered by Mirror UI which provides a select box to list local repos.
However, validation on the server side is a also good idea. I will do it in the f/u work. 😉

@minwoox
Copy link
Contributor

minwoox commented Aug 16, 2024

How about trying mirroring before pushing so that an invalid configuration isn't committed at all?
(We can probably raise an exception when the corresponding credential isn't submitted before the mirror configuration is created.)
I've implemented the k8s service in that way.

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@ikhoon
Copy link
Contributor Author

ikhoon commented Aug 16, 2024

How about trying mirroring before pushing so that an invalid configuration isn't committed at all?

This PR focuses on validating a remote URI. I will do it in the next milestone while implementing #700

@ikhoon ikhoon merged commit eb70014 into line:main Aug 16, 2024
9 of 10 checks passed
@ikhoon ikhoon deleted the fix-mirror-git-url branch August 16, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants