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: add CreatorUsername label when user is signed in via SSO. Fixes… #7109

Merged
merged 2 commits into from
Nov 17, 2021

Conversation

nityanandagohain
Copy link
Contributor

closes #7099

Signed-off-by: Nityananda Gohain nityanandagohain@gmail.com

Don't bother creating a PR until you've done this:

  • Run make pre-commit -B to fix codegen, lint, and commit message problems.

Create your PR as a draft.

  • Your PR needs to pass the required checks before it can be approved. If the check is not required (e.g. E2E tests) it
    does not need to pass.
  • Once required tests have passed, you can make it "Ready for review".
  • Say how how you tested your changes. If you changed the UI, attach screenshots.

Tips:

  • If changes were requested, and you've made them, then dismiss the review to get it looked at again.
  • Add you organization to USERS.md if you like.
  • You can ask for help!

…rgoproj#7099

Signed-off-by: Nityananda Gohain <nityanandagohain@gmail.com>
Signed-off-by: nityanandagohain <nityanandagohain@gmail.com>
@nityanandagohain
Copy link
Contributor Author

@alexec a gentle reminder for the workflow approval, thanks :)

Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

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

If this PR is ready for review, you should mark it as ready on Github :)

@@ -14,6 +14,7 @@ type Claims struct {
Email string `json:"email,omitempty"`
EmailVerified bool `json:"email_verified,omitempty"`
ServiceAccountName string `json:"service_account_name,omitempty"`
Username string `json:"preferred_username,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Why preferred_username instead of username?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since preferred_username is a standard claim that's why used it.

ref https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims

do you think we should give an option to change/override it just like we do for groups i.e customGroupClaimName ?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe change field name to match the JSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we can do that or add to the docs that perferred_username claim will be used for username ?

What would you suggest ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It think qualification is good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, but I am not able to comprehend what you are suggesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @alexec please help me on how should I proceed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think field names should match JSON name, i.e. this should be

PreferredUsername           string                 `json:"preferred_username,omitempty"`

@nityanandagohain nityanandagohain marked this pull request as ready for review November 8, 2021 16:02
LabelKeyCreatorEmail = workflow.WorkflowFullName + "/creator-email"
LabelKeyCreator = workflow.WorkflowFullName + "/creator"
LabelKeyCreatorEmail = workflow.WorkflowFullName + "/creator-email"
LabelKeyCreatorUsername = workflow.WorkflowFullName + "/creator-username"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be honest(?) here? The value is not username is is in fact the user preferred username

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, makes sense. I will make the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made the changes, but will require a workflow approval again.

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

changes requested

Signed-off-by: nityanandagohain <nityanandagohain@gmail.com>
@alexec alexec enabled auto-merge (squash) November 17, 2021 17:09
@alexec alexec merged commit 09987a6 into argoproj:master Nov 17, 2021
@sarabala1979 sarabala1979 mentioned this pull request Mar 1, 2022
@MarahAbu
Copy link

Hi, I'm using argo workflows version v3.2.9 and trying to use the creator username label {{workflow.labels.workflows.argoproj.io/creator-preferred-username}} but its not working although this feature should be implemented in the version i'm using, also in the user info i don't see the preferred username I only see email

@nityanandagohain
Copy link
Contributor Author

If the response from your IDP provider contains preferred_username in claims it will be added to argo JWE.

@MarahAbu can you check if you have added the correct scope in Argo config, if required by your IDP ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal to add CreatorUsername label when user is signed in via SSO
4 participants