-
Notifications
You must be signed in to change notification settings - Fork 25
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
Introduced IdentityClaims embedded type to UserSignup.Spec #373
Conversation
api/v1alpha1/usersignup_types.go
Outdated
// GivenName contains the value of the 'given_name' claim | ||
GivenName string `json:"givenName,omitempty"` | ||
|
||
// FamilyName contains the value of the 'family_name' claim | ||
FamilyName string `json:"familyName,omitempty"` | ||
|
||
// Company contains the value of the 'company' claim | ||
Company string `json:"company,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep these separate. The reason is that the names stay in UserSignup CR only, but the IDs will be copied to MUR & UserAccount CRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to propagate everything from UserSignup.Spec.IdentityClaims to other resources like MUR and UserAccount. We still do it for selected fields like we do now. But having all sso token claims in one place (IdentityClaims) instead of spreading them as we do now is the whole point of this refactoring, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, when we talked last time, we discussed moving only the IDs into the separate struct, we didn't discuss doing that also for the other claims.
The main problem that I wanted to solve was to have one place for all SSO IDs that need to go from the UserSignup into MUR & UserAccount. Right now, we have the IDs at multiple places and it's very chaotic.
So in other words, I wanted to move the IDs from the annotations and spec into the separate field/struct. In this way, as soon as we need to add another ID that need to be propagated down into the UserAccount, we would modify this single struct. Copying that over to MUR & UserAccount would be much easier.
We don't have this problem with the names nor with the company field. we don't need to copy it anywhere, we don't have them stored in annotations either. The only thing we wanted to address wrt names was to keep it up-to-date with SSO.
So I'm slightly confused by what is the purpose of this PR.
Do you want to keep all values from the SSO claims under a separate field. So why not moving the "username" as well? And the email from the annotation? And if we moved everything under identitytokenClaims
field, would be anything left in the spec?
Why not leaving the names where they are and move only the IDs to make the propagation easier?
spec:
familyName:
givenName:
company:
username: mjobanek
idClaims:
sub:
userID:
accountID:
originalSub:
api/v1alpha1/usersignup_types.go
Outdated
// Sub contains the value of the 'sub' claim | ||
Sub string `json:"sub,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't sub the same as userID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This is one of the reason we are doing this refactoring. To get rid of the current confusion when we define the sub
claim as UserID
field. user_id
and sub
claim are different claims. In same cases they have the same value (when we use the sub claim override in some SSO clients) and some they are different (in console.redhat.com client for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now.
But shouldn't it be them moved into the PropagatedClaims section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my point is, currently the claim is mapped to userID which is used for creating Identities in the member clusters - this should stay the same right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently not always.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved sub to the propagated claims, and also made it required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about email and full name? What else we do propagate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In MasterUserRecord we only have UserID and OriginalSub (plus the SSO annotations). I don't see email and/or full name required elsewhere at present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the subject of notifications, I believe that we're only sending them from host operator currently and as far as I'm aware they're only generated off the UserSignup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, good
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that the email is propagated down to UserAccount as well
// Email contains the user's email address | ||
Email string `json:"email"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, we do propagate the email as well - I see that in MUR as well as in UserAccount as an annotation - this should be moved to PropagatedClaims
Description
This PR introduces a new IdentityClaims type to store as-is values of certain claims from the identity provider.
Fixes https://issues.redhat.com/browse/SANDBOX-324
Checks
Did you run
make generate
target? yesDid
make generate
change anything in other projects (host-operator, member-operator)? yesIn case of new CRD, did you the following? yes
resources/setup/roles/host.yaml
in the sandbox-sre repositoryPROJECT
file: https://github.com/codeready-toolchain/host-operator/blob/master/PROJECTCSV
file: https://github.com/codeready-toolchain/host-operator/blob/master/config/manifests/bases/host-operator.clusterserviceversion.yamlIn case other projects are changed, please provides PR links.