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: glob support for RedirectURIs #297

Merged
merged 3 commits into from
Mar 6, 2023
Merged

Conversation

muir
Copy link
Contributor

@muir muir commented Mar 1, 2023

To maintain backwards compatibility, an optional op.Client interface is added:

// HasRedirectGlobs is an optional interface that can be implemented by implementors of
// Client. See https://pkg.go.dev/github.com/gobwas/glob#Compile for glob
// interpretation. Redirect URIs that match either the non-glob version or the
// glob version will be accepted. Glob URIs are only partially supported for native
// clients: "http://" is not allowed except for loopback or in dev mode.
type HasRedirectGlobs interface {
	RedirectURIGlobs() []string
	PostLogoutRedirectURIGlobs() []string
}

In the next branch, instead of adding an additional interface, RedirectURIs will be changed to return two arrays.

Resolves #293

@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Merging #297 (2df16c3) into main (815ced4) will decrease coverage by 0.13%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main     #297      +/-   ##
==========================================
- Coverage   39.75%   39.63%   -0.13%     
==========================================
  Files          74       74              
  Lines        5941     5965      +24     
==========================================
+ Hits         2362     2364       +2     
- Misses       3329     3350      +21     
- Partials      250      251       +1     
Impacted Files Coverage Δ
example/server/storage/client.go 86.36% <0.00%> (-4.12%) ⬇️
pkg/op/client.go 100.00% <ø> (ø)
pkg/op/session.go 32.91% <0.00%> (-4.24%) ⬇️
pkg/op/auth_request.go 53.58% <55.00%> (-1.03%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -98,5 +100,16 @@ func ValidateEndSessionPostLogoutRedirectURI(postLogoutRedirectURI string, clien
return nil
}
}
if globClient, ok := client.(HasRedirectGlobs); ok {
for _, uriGlob := range globClient.PostLogoutRedirectURIGlobs() {
matcher, err := glob.Compile(uriGlob)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you aware of https://github.com/gobwas/glob#performance?

I understand that this is the only moment where you can actually compile the globs, as they come from storage and there is no way to store the compiled objects. However, I do would like to see some benchmarks to have an idea about performance impact using this library / approach. It might be just fine, but this sentence triggers me:

This means, that compilation could take time, but strings matching is done faster,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@muhlemmer I could add a LRU cache of compiled globs. Or perhaps, I should add that to the glob library so that all users benefit? Adding a cache is pretty easy but adds a dependency and a utility function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The nice LRU cache that is already indirectly part of the oidc package, github.com/hashicorp/golang-lru, has a nice generics-enhanced interface. Are we ready to use generics in the next branch?

@muhlemmer
Copy link
Collaborator

Thanks for working on this! I quickly went over what you've done so far and left a comment. When this PR moves out of draft I will do a more detailed review.

Unless you have a direct need to have this in current stable main, I would suggest to go directly for next. Zitadel is already running on a predecessor of next for months now and we intend to merge it into main within the next 2 weeks. (yes, there is still much refactoring to be done, but probably we just go faster to v3).

@muir
Copy link
Contributor Author

muir commented Mar 1, 2023

Thanks for working on this! I quickly went over what you've done so far and left a comment. When this PR moves out of draft I will do a more detailed review.

Unless you have a direct need to have this in current stable main, I would suggest to go directly for next. Zitadel is already running on a predecessor of next for months now and we intend to merge it into main within the next 2 weeks. (yes, there is still much refactoring to be done, but probably we just go faster to v3).

Hmm. Do you like my idea for next of having RedirectURIs return two slices?

Are there releases of the next branch that I can reference?

Answer: Yes: "v2.0.0-dynamic-issuer.8" I can switch to next easily enough.

@muir muir marked this pull request as ready for review March 1, 2023 23:41
@muhlemmer
Copy link
Collaborator

@muhlemmer
Copy link
Collaborator

I'm okay with merging this into main because it would solve #293 on the short term. However, for next I want to investigate if we can find a solution without API or storage change and double slice returns. So don't send a PR for that. TBD in follow-up issue

@muhlemmer muhlemmer merged commit 7e57985 into zitadel:main Mar 6, 2023
@github-actions
Copy link

github-actions bot commented Mar 6, 2023

🎉 This PR is included in version 1.13.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

Wildcard URIs are not valid redirect URIs
2 participants