Skip to content

feat: implement bitbucket-server external auth defaults #10520

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
Merged
merged 8 commits into from
Nov 8, 2023
Merged

feat: implement bitbucket-server external auth defaults #10520

merged 8 commits into from
Nov 8, 2023

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Nov 3, 2023 •
edited
Loading

What this does

Bitbucket cloud (SaaS cloud) != Bitbucket server (self hosted)

We need reasonable defaults for bitbucket-server. Using bitbucket cloud defaults is not only confusing, but incorrect.

This sets the correct oauth urls based on the AUTH_URL. Sets:

  • CODER_EXTERNAL_AUTH_1_TOKEN_URL=<url>/rest/oauth2/latest/token
  • CODER_EXTERNAL_AUTH_1_VALIDATE_URL=<url>/rest/api/latest/inbox/pull-requests/count
  • CODER_EXTERNAL_AUTH_1_SCOPES="PUBLIC_REPOS,REPO_READ,REPO_WRITE"
  • CODER_EXTERNAL_AUTH_1_REGEX=^(https?://)?<host>(/.*)?$

Minimum env vars

export CODER_EXTERNAL_AUTH_0_TYPE="bitbucket-server"
export CODER_EXTERNAL_AUTH_0_ID="bitbucket"
export CODER_EXTERNAL_AUTH_0_CLIENT_ID=<client_id>
export CODER_EXTERNAL_AUTH_0_CLIENT_SECRET=<client_secret>
export CODER_EXTERNAL_AUTH_0_AUTH_URL=<auth_url>

Before TOKEN_URL, VALIDATE_URL, SCOPES, and REGEX were all required.

@Emyrk
feat: implement bitbucket-server external auth defaults
f7a5742 
Bitbucket cloud != Bitbucket server
Add reasonable defaults for server
@Emyrk Emyrk marked this pull request as ready for review November 3, 2023 20:30
ericpaulsen
ericpaulsen approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@ericpaulsen ericpaulsen left a comment

Choose a reason for hiding this comment

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

I'll let @spikecurtis review the go code, but the Bitbucket configuration variables look good.

kylecarbs
kylecarbs requested changes Nov 3, 2023
View reviewed changes
coderd/externalauth/externalauth.go Outdated
@@ -493,8 +494,27 @@ func ConvertConfig(entries []codersdk.ExternalAuthConfig, accessURL *url.URL) ([
}

// applyDefaultsToConfig applies defaults to the config entry.
func applyDefaultsToConfig(config *codersdk.ExternalAuthConfig) {
defaults := defaults[codersdk.EnhancedExternalAuthProvider(config.Type)]
func configDefaults(config *codersdk.ExternalAuthConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

Why shorten the name? configDefaults seems like it could return defaults, whereas apply states that it'd apply to the arg.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed. I just had to make another func that actually applies the defaults. See new names of applyDefaultsToConfig and copyDefaultSettings(config *codersdk.ExternalAuthConfig, defaults codersdk.ExternalAuthConfig)

coderd/externalauth/externalauth.go

// Populate Regex, ValidateURL, and TokenURL.
// Default regex should be anything using the same host as the auth url.
defaults.Regex = fmt.Sprintf(`^(https?://)?%s(/.*)?$`, strings.ReplaceAll(auth.Host, ".", `\.`))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we be doing these for all of them? Seems like making it just for BitBucket server is weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps the self-hosted ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do.

Azure

coder/coderd/externalauth/externalauth.go

Line 607 in 84509a1

Regex: `^(https?://)?dev\.azure\.com(/.*)?$`,

Bitbucket Cloud

coder/coderd/externalauth/externalauth.go

Line 616 in 84509a1

Regex: `^(https?://)?bitbucket\.org(/.*)?$`,

Gitlab

coder/coderd/externalauth/externalauth.go

Line 625 in 84509a1

Regex: `^(https?://)?gitlab\.com(/.*)?$`,

Github

coder/coderd/externalauth/externalauth.go

Line 634 in 84509a1

Regex: `^(https?://)?github\.com(/.*)?$`,

Bitbucket Server is always hosted on a different domain for each customer. So it's a dynamic regex string based on the AUTH_URL. The other providers are all cloud based with static urls.

Copy link
Member Author

Choose a reason for hiding this comment

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

@spikecurtis yes, the self hosted ones need this dynamic approach. We need to handle them case by case. Maybe there will be generalizations, but for now I only implemented self hosted bitbucket server.

Gitlab for example can definitely have some broken defaults right now.

Copy link
Member

Choose a reason for hiding this comment

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

@Emyrk automatically doing the Regex should be a global change in my mind; it's weird to do it for a single provider.

Copy link
Member Author

Choose a reason for hiding this comment

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

The regex is automatic for all providers. As in, the default values are correct for every type now

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked to @kylecarbs offline a bit.

Yes this problem exists with Gitlab and Github Enterprise. We should probably have some solution for these.

I do not want to break backwards compatibility, and I do not have the test environments to confirm those (eg, are the urls consistent? bitbucket cloud and server are different).

So for this PR, I'd rather not try and solve github enterprise and gitlab self hosted.

kylecarbs
kylecarbs reviewed Nov 6, 2023
View reviewed changes
@Emyrk Emyrk requested a review from kylecarbs November 7, 2023 14:46
kylecarbs
kylecarbs reviewed Nov 8, 2023
View reviewed changes
coderd/externalauth/externalauth.go
Comment on lines +502 to +506
defer func() {
// The config type determines the config ID (if unset). So change the legacy
// type to the correct new type after the defaults have been configured.
config.Type = string(codersdk.EnhancedExternalAuthProviderBitBucketCloud)
}()
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused why we need to do this when we set it above.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because the actual string "bitbucket" is being used in the copyDefaultSettings function, so for legacy reasons it has to be the legacy value. But at the end of the function, we need to set it to the new type. And I just did it in a defer to not put the if statement in 2 places.

@Emyrk Emyrk merged commit aded7b1 into main Nov 8, 2023
@Emyrk Emyrk deleted the stevenmasley/bitbucket_server_defaults branch November 8, 2023 17:05
@github-actions github-actions bot locked and limited conversation to collaborators Nov 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kylecarbs kylecarbs kylecarbs approved these changes

@ericpaulsen ericpaulsen ericpaulsen approved these changes

@spikecurtis spikecurtis Awaiting requested review from spikecurtis

Assignees

@Emyrk Emyrk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Emyrk @spikecurtis @kylecarbs @ericpaulsen