COR-475: identity #409
COR-475: identity #409
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: internetti The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| err2 := loginStore.CreateOidcIdentityProfile(newIdentityProfile) | ||
| if err2 != nil { | ||
| l.Error("failed to store identity profile", zap.Error(err2)) | ||
| } |
There was a problem hiding this comment.
| err2 := loginStore.CreateOidcIdentityProfile(newIdentityProfile) | |
| if err2 != nil { | |
| l.Error("failed to store identity profile", zap.Error(err2)) | |
| } | |
| if err := loginStore.CreateOidcIdentityProfile(newIdentityProfile); err != nil { | |
| l.Error("failed to store identity profile", zap.Error(err)) | |
| return err | |
| } |
| } else { | ||
| l.Error("failed to get user identifier for oidc provider", zap.Error(err)) |
There was a problem hiding this comment.
I feel like I had a reason but I cannot remember it for the life of me, so let's say no 😅
| var userProfile authrequest.Claims | ||
|
|
||
| var mappedClaimInft map[string]interface{} | ||
| mappedClaimInft, ok = claimInft.(map[string]interface{}) | ||
|
|
||
| subject, ok := mappedClaimInft[idp.ClaimMappings.Subject].(string) | ||
| if ok { | ||
| userProfile.Subject = subject | ||
| } | ||
| displayName, ok := mappedClaimInft[idp.ClaimMappings.DisplayName].(string) | ||
| if ok { | ||
| userProfile.DisplayName = displayName | ||
| } | ||
| fullName, ok := mappedClaimInft[idp.ClaimMappings.FullName].(string) | ||
| if ok { | ||
| userProfile.FullName = fullName | ||
| } | ||
| email, ok := mappedClaimInft[idp.ClaimMappings.Email].(string) | ||
| if ok { | ||
| userProfile.Email = email | ||
| } | ||
| emailVerified, ok := mappedClaimInft[idp.ClaimMappings.EmailVerified].(bool) | ||
| if ok { | ||
| userProfile.EmailVerified = emailVerified | ||
| } |
There was a problem hiding this comment.
Could we extract this into a function ? Like extractIdentityProfile(claims, idp)
Also I think this would for sure need some testing
| package store | ||
|
|
||
| type IdentityProfile struct { | ||
| ID string |
There was a problem hiding this comment.
We probably need either a primary key on ID, or a primary key on IdentityProviderID and Subject
There was a problem hiding this comment.
I thought gorm uses ID as primary key by default?
|
|
||
| db, err := l.db.Get() | ||
| if err != nil { | ||
| return err |
| } | ||
|
|
||
| if err := db.Create(&profile).Error; err != nil { | ||
| return err |
pkg/server/login/store/interface.go
Outdated
| FindOidcIdentifier(identifier string, identityProviderId string) (*CredentialIdentifier, error) | ||
| CreateOidcIdentity(issuer string, identifier string, initialAccessToken string, initialRefreshToken string, initialIdToken string) (*Identity, error) | ||
| CreateOidcIdentityProfile(profile IdentityProfile) error |
There was a problem hiding this comment.
We need to add a ctx as the first argument of all of those methods
internetti
force-pushed
the
COR-475_identity
branch
from
aa906fb to
162b9ca
Compare
| idp: idp2, | ||
| expect: authrequest.Claims{ | ||
| Subject: "SubjectFieldName", | ||
| DisplayName: "", | ||
| FullName: "", | ||
| Email: "", | ||
| EmailVerified: false, | ||
| }, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "fills profile with placeholders for missing data", | ||
| claims: map[string]interface{}{ | ||
| "SubjectFieldName": "subjectValue", | ||
| }, | ||
| expect: authrequest.Claims{ | ||
| Subject: "subjectValue", | ||
| DisplayName: "<no value> <no value> <no value>", | ||
| FullName: "<no value> <no value>", | ||
| Email: "<no value>", | ||
| EmailVerified: false, | ||
| }, | ||
| wantErr: false, |
There was a problem hiding this comment.
I'd actually expect errors in these cases, but the template execute function doesn't throw any, is it worth testing for this manually and throwing the errors myself?
|
Kudos, SonarCloud Quality Gate passed!
|
COR-475
introduced fix structure for claims to map to
in agreement with ludo, a new table is being created to store the claim data