Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

auth: auth-proxy-http-header command for testing multiple users#39231

Merged
keegancsmith merged 7 commits into
mainfrom
k/20220721-auth-proxy
Jul 27, 2022
Merged

auth: auth-proxy-http-header command for testing multiple users#39231
keegancsmith merged 7 commits into
mainfrom
k/20220721-auth-proxy

Conversation

@keegancsmith

Copy link
Copy Markdown
Member

For permission testing I wrote this command to very conveniently switch between users. This is kind of like the already existing testproxy, except much more convenient to use as well as being targetted for auth testing rather than http-header testing.

I'm unsure of how to document this further so people are aware of it. Alternatively I think it is also useful to maybe spin up by default in our enterprise env. I'll leave both of those for future PRs. For now I will advertise in #dev-chat and #dev-experience.

Here is the example output to give you a feel for what it does:

$ go run ./internal/cmd/auth-proxy-http-header
https://docs.sourcegraph.com/admin/auth#http-authentication-proxies

  "auth.providers": [
    {
      "type": "http-header",
      "usernameHeader": "X-Forwarded-User",
      "emailHeader": "X-Forwarded-Email"
    }
  ]

Visit http://127.0.0.1:10810 for keegan keegan@sourcegraph.com
Visit http://127.0.0.1:10811 for user1 keegan+user1@sourcegraph.com
Visit http://127.0.0.1:10812 for user2 keegan+user2@sourcegraph.com
Visit http://127.0.0.1:10813 for user3 keegan+user3@sourcegraph.com
Visit http://127.0.0.1:10814 for user4 keegan+user4@sourcegraph.com
Visit http://127.0.0.1:10815 for user5 keegan+user5@sourcegraph.com

Test Plan: Ran locally

For permission testing I wrote this command to very conveniently switch
between users. This is kind of like the already existing testproxy,
except much more convenient to use as well as being targetted for auth
testing rather than http-header testing.

I'm unsure of how to document this further so people are aware of it.
Alternatively I think it is also useful to maybe spin up by default in
our enterprise env. I'll leave both of those for future PRs. For now I
will advertise in #dev-chat and #dev-experience.

Here is the example output to give you a feel for what it does:

  $ go run ./internal/cmd/auth-proxy-http-header
  https://docs.sourcegraph.com/admin/auth#http-authentication-proxies

    "auth.providers": [
      {
        "type": "http-header",
        "usernameHeader": "X-Forwarded-User",
        "emailHeader": "X-Forwarded-Email"
      }
    ]

  Visit http://127.0.0.1:10810 for keegan keegan@sourcegraph.com
  Visit http://127.0.0.1:10811 for user1 keegan+user1@sourcegraph.com
  Visit http://127.0.0.1:10812 for user2 keegan+user2@sourcegraph.com
  Visit http://127.0.0.1:10813 for user3 keegan+user3@sourcegraph.com
  Visit http://127.0.0.1:10814 for user4 keegan+user4@sourcegraph.com
  Visit http://127.0.0.1:10815 for user5 keegan+user5@sourcegraph.com

Test Plan: Ran locally
@keegancsmith keegancsmith requested review from a team July 21, 2022 13:23
@cla-bot cla-bot Bot added the cla-signed label Jul 21, 2022
@keegancsmith keegancsmith added the dx Issues and PRs related to developer experience concerns label Jul 21, 2022
@jhchabran

Copy link
Copy Markdown
Contributor

Damn, this is awesome.

@sourcegraph-bot

sourcegraph-bot commented Jul 21, 2022

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff cd1391b...b130b8d.

Notify File(s)
@unknwon enterprise/cmd/frontend/internal/auth/httpheader/middleware.go
enterprise/cmd/frontend/internal/auth/httpheader/testproxy.go

@keegancsmith keegancsmith force-pushed the k/20220721-auth-proxy branch from 28b9bee to 220b075 Compare July 21, 2022 13:30

@jhchabran jhchabran left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What a great example of stuff we can build with httputil.ReverseProxy 😮🤘

Yes, I think we'll need a follow-up PR to make sure teammates are aware of this, otherwise I'm worried about this jewel would go unnoticed. I'll be happy to do it.

@keegancsmith

Copy link
Copy Markdown
Member Author

FYI I am adding this command to the allow list of stdlib log. I think the stdlib log is much more readable for humans, and this is intended to only be used internally by devs.

@sourcegraph-bot

sourcegraph-bot commented Jul 21, 2022

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in OWNERS files for diff cd1391b...b130b8d.

No notifications.

@bobheadxi

Copy link
Copy Markdown
Member

FYI I am adding this command to the allow list of stdlib log. I think the stdlib log is much more readable for humans, and this is intended to only be used internally by devs.

Should this tool be in dev/internal or similar instead? I think there we don't enforce this restriction by default, and it might make sense for a tool like this

@keegancsmith

Copy link
Copy Markdown
Member Author

FYI I am adding this command to the allow list of stdlib log. I think the stdlib log is much more readable for humans, and this is intended to only be used internally by devs.

Should this tool be in dev/internal or similar instead? I think there we don't enforce this restriction by default, and it might make sense for a tool like this

That could work, but want something that is discoverable. Right now everything under ./internal/cmd would also make sense to be under dev/internal/cmd. FYI your linter skips over them since most of those commands have there own go.mod.

I won't merge just yet, just want your heads up what you think makes the most sense with that bit of info.

@unknwon unknwon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice :)

@jhchabran

Copy link
Copy Markdown
Contributor

That could work, but want something that is discoverable.

I'm not sure to understand, how moving things into dev/internal from internal/cmd would change how discoverable it is? Isn't discoverability something that we should address within sg itself?

@keegancsmith

Copy link
Copy Markdown
Member Author

dev/internal/cmd doesn't exist. But yeah good point. I'll rename.

@keegancsmith keegancsmith enabled auto-merge (squash) July 27, 2022 07:56
@keegancsmith keegancsmith merged commit bb1823a into main Jul 27, 2022
@keegancsmith keegancsmith deleted the k/20220721-auth-proxy branch July 27, 2022 08:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed dx Issues and PRs related to developer experience concerns

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants