coreos / go-oidc

A Go OpenID Connect client.
Apache License 2.0
1.93k stars 393 forks source link

oidc.Identity is too limited; does not allow for setting additional attributes related to Identity. #83

Closed clc-github closed 7 years ago

clc-github commented 8 years ago

oidc.Identity is too limited in functionality. Without replacing it as a type, users of go-oidc not able to add additional attributes to an Identity.

I need to modify dex to add Additional Claims to the JWT. To accomplish this, I replaced the oidc.Identity type in dex with a new dex specific type. The result turned out to be oidc.Identity with one additional field.

I don't see any advantages to doing the mechanical work of replacing the type within dex. I also assume any other users of the go-oidc package will eventually run into a similar issue. So rather than continue down the path of looking at replacing oidc.Identity in dex, I would like to make the existing type less limited.

ericchiang commented 8 years ago

First, I'm okay with adding additional claims to the identity struct. But I'd also like to comment that extremely easy to do this yourself.

type MyIdentity struct {
    oidc.Identity
    AdditionalClaims oidc.Claims
}

Is there a specific part of dex code you could point to that demonstrates why this it's necessary to add this in this package?

clc-github commented 8 years ago

On the advice of Bobby Rullo, I tried adding a new type to dex first. It is straight forward, but it turned out to just be a full 100% replacement of oidc.Identity with the new type.

This is needed because for the AdditionalClaims from the dex connector to be useful, they need to be persisted in the PostgreSQL session.identity column at a minimum (ignoring refresh token usage).

ex:

-[ RECORD 1 ]+------------------------------------------------------------------------------------------------------------------------------------------------------------
id           | RfslpxgaeSc=
state        | EXCHANGED
created_at   | 1463089253
expires_at   | 1463132453
client_id    | B7m2tUu2hOguYFQ4rrPa6Kg79PtdvTdAYAjccb6Kg84=@10.9.8.7
client_state | 
redirect_url | http://10.9.8.7:32100/callback
identity     | {"ID":"74d53884-14d0-408c-bb42-7c13e022800c","Name":"","Email":"","ExpiresAt":"0001-01-01T00:00:00Z","AdditonalClaims":{"roles":["ONE","TWO"],"roles2":"THREE"}}
connector_id | local
user_id      | 74d53884-14d0-408c-bb42-7c13e022800c
register     | f
nonce        | 
scope        | openid email profile offline_access

Once the type is changed on the IdentityProvider interface and the Session struct, it needs to be replaced almost everywhere.

Given the definition of Identity from the OIDC core spec, seemed to make much more sense to modify oidc.Identity rather than replace it.

Here is list of files (no unit test changes) that I change on my POC branch:

$ git diff --name-only master
connector/connector_bitbucket.go
connector/connector_github.go
connector/connector_ldap.go
connector/connector_local.go
connector/connector_oauth2.go
connector/connector_oidc.go
connector/interface.go
connector/login_local.go
db/session.go
server/register.go
server/server.go
session/manager/manager.go
session/session.go
ericchiang commented 8 years ago

We've started to move away from using structs defined in this package for storage in dex. See https://github.com/coreos/dex/pull/411.

I'd actually much rather prefer an Identity struct in dex that's tailored to what dex actually provides. Particularly when this comes to custom fields. It avoids using interfaces for those not directly supported by OpenID Connect, and allows dex to continue changing without having to change a public facing package.

ericchiang commented 7 years ago

The new client no longer has the concept of an oidc "Identity"