Added back channel logout endpoint without context#4704
Added back channel logout endpoint without context#4704
Conversation
shepilov
force-pushed
the
feat/backchannel-logout-without-context
branch
4 times, most recently
from
8f9301e to
1200bcf
Compare
| } | ||
| } | ||
|
|
||
| func combineLogoutContextCandidates(contextsBySID, contextsByIssuerAudience []string) []string { |
There was a problem hiding this comment.
The naming of these parameters is confusing as I expect to find maps but they're slices.
There was a problem hiding this comment.
What name do you propose for the list of contexts that were found by sid ?
web/oidc/oidc.go
Outdated
| var contextsBySID []string | ||
| var err error | ||
| if sid != "" { | ||
| contextsBySID, err = session.FindOIDCProviderKeysBySID(sid) |
There was a problem hiding this comment.
Confusing variable name. I expect it to be a map while it's a slice.
web/oidc/oidc.go
Outdated
| // if there is no session, try to resolve it by OIDC configuration in logout token | ||
| case 0: | ||
| { | ||
| contextsByIssuerAudience := findOIDCContextsByIssuerAudience(iss, aud) |
There was a problem hiding this comment.
Confusing variable name. I expect it to be a map while it's a slice.
| func Logout(c echo.Context) error { | ||
| contextName := c.Param("context") | ||
| conf, err := getGenericConfig(contextName) | ||
| return handleLogout(c, c.Param("context")) |
There was a problem hiding this comment.
Why put the implementation in a separate function since it's used only here?
There was a problem hiding this comment.
Initially, I was going to move it to a separate package, but it's just a small isolation of the web layer from logic/service. But maybe it doesnt make sense now
| return &oidcConfig, nil | ||
| } | ||
| // OIDCConfiguration represents the OpenID Connect configuration from the well-known endpoint. | ||
| type OIDCConfiguration = oidcprovider.Metadata |
There was a problem hiding this comment.
Why the alias? It would be easier to follow if we used oidcprovider.Metadata directly.
There was a problem hiding this comment.
refactoring artifacts
| } | ||
|
|
||
| s := &Session{} | ||
| err = couchdb.GetDoc(inst, consts.Sessions, ref.SessionID, s) |
There was a problem hiding this comment.
Why not use Get(inst, ref.SessionID) here?
There was a problem hiding this comment.
because Get(inst, ref.SessionID) it's not just document fetch, it's doing a lot of things, and for example, we might refresh LastSeen on a session we are about to delete
shepilov
force-pushed
the
feat/backchannel-logout-without-context
branch
from
1200bcf to
3005b31
Compare
2 participants
The previous design had three practical problems:
With this PR: