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(oidc): support generic OIDC providers #1223

Merged
merged 7 commits into from
Dec 14, 2022
Merged

Conversation

GeorgeMac
Copy link
Contributor

This is a PR based on the gm/oidc-google branch. I thought I would propose it here first.
If we like it, we can merge it into that and then merge that into main.

This makes the whole OIDC package generic for multiple providers.
The providers key is simply a map of any user-defined provider name to provider config.

This leads to dynamic OIDC endpoints. e.g.

authentication:
  methods:
    oidc:
      providers:
        foo:
          // ...

This will lead to the following endpoints:

  • /auth/v1/method/oidc/foo/authorize
  • /auth/v1/method/oidc/foo/callback

I did a run through with Google, and it works as normal.

@GeorgeMac GeorgeMac requested a review from a team as a code owner December 13, 2022 20:12
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

makes sense to me! looks good!!

two minor comments

internal/config/authentication.go Outdated Show resolved Hide resolved
// time.
type trie map[string]trie

func (t trie) getPrefix(parts ...string) (trie, bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we add some more tests around this method and the function below it if possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've taken another pass in a slightly different way. Which also allowed me to test it more thoroughly.
It also accounts for something this implementation couldn't do. Which was keys in maps with _ in them.
See what you think.

@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2022

Codecov Report

Merging #1223 (bd23cf6) into gm/oidc-google (ba7da0d) will increase coverage by 0.19%.
The diff coverage is 91.66%.

@@                Coverage Diff                 @@
##           gm/oidc-google    #1223      +/-   ##
==================================================
+ Coverage           79.61%   79.81%   +0.19%     
==================================================
  Files                  40       40              
  Lines                3017     3076      +59     
==================================================
+ Hits                 2402     2455      +53     
- Misses                496      499       +3     
- Partials              119      122       +3     
Impacted Files Coverage Δ
internal/server/auth/method/oidc/http.go 85.55% <ø> (-0.62%) ⬇️
internal/server/auth/method/oidc/server.go 67.52% <82.05%> (-1.29%) ⬇️
internal/config/config.go 86.82% <97.01%> (+4.52%) ⬆️
internal/config/authentication.go 83.87% <100.00%> (+0.26%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

🕺

@GeorgeMac GeorgeMac merged commit 2714a8e into gm/oidc-google Dec 14, 2022
@GeorgeMac GeorgeMac deleted the gm/oidc-generic branch December 14, 2022 17:53
GeorgeMac added a commit that referenced this pull request Dec 22, 2022
* refactor(main): move grpc and http composition into internal/cmd

* chore(internal/cmd): unexport grpc register types

* chore(cmd/grpc): rename pushShutdown onShutdown and move clientConn to main

* feat(oidc): implement Google OIDC provider

feat(auth/oidc): implement authorize and callback

refactor(oidc): switch to hashicorp/cap package

test(oidc): add extra logging context for failure in CI

chore: more test logging context

chore: logging context around client cookie jar contents

chore: print entire cookie jar

fix(oidc): test use localhost over ip for cookie jar interactions

chore: appears the linters

refactor(internal/cmd): flatten bootstrapping package into one

* chore(method/oidc): improve docs around http middleware

* refactor(errors): add generic errors.NewErrorf

* test(oidc): increase coverage around invalid state parameter on callback

* chore: remove excess newline

* fix(oidc): install missing auth middleware

* feat(oidc): support generic OIDC providers (#1223)

* feat(oidc): support generic OIDC providers

* refactor(config): support maps with keys containing underscore

* chore: appease the linters

* chore(config): update doc string

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

* chore(config): update bind function doc

* feat(oidc): add more standard OIDC claims

* chore(oidc): remove redundent return statement

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

* chore(errors): restore Errorf helper functions

* chore(errors): switch back to Errorf utility functions

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants