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

(feat) add ability to allow anonymous usage mode based on "auth.public" site config and "allow-anonymous-usage" license tag #52440

Merged
merged 2 commits into from May 26, 2023

Conversation

erzhtor
Copy link
Contributor

@erzhtor erzhtor commented May 25, 2023

Closes #52439.

Test plan

  • sg start dotcom and generate license with allow-anonymous-usage tag
  • Restart in enterprise mode sg start enterprise and set newly created license
  • Set auth.public=true in site config
  • Check that instance allows searching and browsing public repositories

Demo

Screen.Recording.2023-05-25.at.15.18.24.mov

@erzhtor erzhtor requested review from ryphil, dadlerj and a team May 25, 2023 09:28
@erzhtor erzhtor self-assigned this May 25, 2023
@cla-bot cla-bot bot added the cla-signed label May 25, 2023
@@ -16,6 +16,9 @@ const (
InternalTag = "internal"
// DevTag denotes licenses used in development environments
DevTag = "dev"
// AllowAnonymousUsageTag denotes licenses that allow anonymous usage, a.k.a public access to the instance
// Warning: This should be used with care and only at special, probably trial/poc stages with customers
AllowAnonymousUsageTag = "allow-anonymous-usage"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: this is a new special license tag that is required to allow auth.public site config to take effect.

@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented May 25, 2023

Codenotify: Notifying subscribers in CODENOTIFY files for diff 35c3b8f...89fa487.

Notify File(s)
@unknwon enterprise/cmd/frontend/internal/auth/BUILD.bazel
enterprise/cmd/frontend/internal/auth/confauth/BUILD.bazel
enterprise/cmd/frontend/internal/auth/confauth/middleware.go
enterprise/cmd/frontend/internal/auth/confauth/middleware_test.go
enterprise/cmd/frontend/internal/auth/init.go
enterprise/internal/licensing/tags.go

func (svc) Start(ctx context.Context, observationCtx *observation.Context, ready service.ReadyFunc, config env.Config) error {
return frontend_shared.CLIMain(ctx, observationCtx, ready, EnterpriseSetupHook)
return frontend_shared.CLIMain(ctx, observationCtx, ready, EnterpriseSetupHook, extraContextMiddleware)
Copy link
Contributor Author

@erzhtor erzhtor May 25, 2023

Choose a reason for hiding this comment

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

FYI: I had to create an extra middleware that is passed from enterprise code, which will set certain context values. This is used because currently the code which controls access in regular oss code, whereas licensing is located in enterprise. So enterprise talks to main access control logic via context.

Perhaps, not the nicest code, but this is the only easy way if we want to guard this new feature based on license tags. Other proper ways will require way more refactoring, which I don't think worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kopancek, I found a better approach using existing auth.RegisterMiddelwares instead of drilling down new extra middleware argument 🙌 . Also, added unite tests. I would appreciate it if you could re-take a look.

Copy link
Contributor

@kopancek kopancek left a comment

Choose a reason for hiding this comment

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

The approach looks good to me, would add some tests for the changed logic, especially the AllowAnonymousRequest and the new middleware.

Comment on lines 127 to 130
if checkAllowAnonymousRequest(req) {
return true
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we combine this with the other clauses? Also it seems like checkAllowAnonymousRequest is already doing the conf.AuthPublic check... E.g. something like:

Suggested change
if checkAllowAnonymousRequest(req) {
return true
}
return checkAllowAnonymousRequest(req) || strings.HasPrefix(req.URL.Path, "/.assets/") || defaultCode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I like to keep clauses separate for simplicity purposes, as combining them will make reading it harder. Besides all the rest of the existing clauses follow the same convention of being separate.

@erzhtor erzhtor requested a review from a team May 25, 2023 10:21
…c" site config and "allow-anonymous-usage" license tag
@erzhtor erzhtor force-pushed the erzhtor/add-ability-to-allow-anonymous-usage branch 2 times, most recently from f441e8c to 616f026 Compare May 25, 2023 16:37
@erzhtor erzhtor requested a review from kopancek May 25, 2023 16:40
@erzhtor erzhtor force-pushed the erzhtor/add-ability-to-allow-anonymous-usage branch from 616f026 to 89fa487 Compare May 25, 2023 16:47
@erzhtor erzhtor merged commit d8a8e5b into main May 26, 2023
11 checks passed
@erzhtor erzhtor deleted the erzhtor/add-ability-to-allow-anonymous-usage branch May 26, 2023 10:57
@github-actions
Copy link
Contributor

The backport to 5.0 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-5.0 5.0
# Navigate to the new working tree
cd .worktrees/backport-5.0
# Create a new branch
git switch --create backport-52440-to-5.0
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d8a8e5bedb08073227ac01589c3eafa4268ec590
# Push it to GitHub
git push --set-upstream origin backport-52440-to-5.0
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-5.0

Then, create a pull request where the base branch is 5.0 and the compare/head branch is backport-52440-to-5.0.

@github-actions github-actions bot added backports failed-backport-to-5.0 release-blocker Prevents us from releasing: https://about.sourcegraph.com/handbook/engineering/releases labels May 26, 2023
erzhtor added a commit that referenced this pull request May 26, 2023
…c" site config and "allow-anonymous-usage" license tag (#52440)

(cherry picked from commit d8a8e5b)
keegancsmith pushed a commit that referenced this pull request May 26, 2023
… on "auth.publi… (#52506)

Closes #52439. Original
PR #52440.

## Test plan
- `sg start dotcom` and generate license with `allow-anonymous-usage`
tag
- Restart in enterprise mode `sg start enterprise` and set newly created
license
- Set `auth.public=true` in site config
- Check that instance allows searching and browsing public repositories

## Demo

https://github.com/sourcegraph/sourcegraph/assets/6717049/344f9b1e-f54a-4148-9bc8-f0f3d3bb4cd7
erzhtor added a commit that referenced this pull request Jun 1, 2023
ErikaRS pushed a commit that referenced this pull request Jun 22, 2023
…c" site config and "allow-anonymous-usage" license tag (#52440)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backports cla-signed release-blocker Prevents us from releasing: https://about.sourcegraph.com/handbook/engineering/releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to allow anonymous usage based on "auth.public" and special license tag
4 participants