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.

Sign up for GitHub

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

Jump to bottom

Use username as token subject in STS login #7637

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Use username as token subject in STS login #7637

merged 1 commit into from
Apr 9, 2024

Conversation

guy-har
Copy link
Contributor

@guy-har guy-har commented Apr 8, 2024 •
edited
Loading

Description

Currently, the STS login endpoint uses the externalID as the subject instead of the username. The token subject should be the username

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 8, 2024

No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged

@guy-har guy-har added include-changelog PR description should be included in next release changelog minor-change Used for PRs that don't require issue attached labels Apr 8, 2024
@guy-har guy-har changed the title Use username as subject Use username as token subject in STS login Apr 8, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 8, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

10 passed

@arielshaqed
Copy link
Contributor

I don't understand:

The token subject should be the username

Why? I would expect the name of the user to be mutable, whereas the external user ID is literally the identity of the user and should be immutable. I can certainly understand adding the name of the user to the token. Why does it have to be the subject?

@Jonathan-Rosenberg
Copy link
Contributor

@arielshaqed I agree, yet the User model doesn't have a user ID

Jonathan-Rosenberg
Jonathan-Rosenberg approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@Jonathan-Rosenberg Jonathan-Rosenberg left a comment

Choose a reason for hiding this comment

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

one day we'll have IDs

@guy-har
Copy link
Contributor Author

guy-har commented Apr 8, 2024

@arielshaqed, the middleware currently expects the subject to be username, which is also the subject that is used in the login flow. It might not be ideal, but the purpose of this PR is to align sts with the login flow

@arielshaqed
Copy link
Contributor

Thanks! Not in any way blocking this PR.

@guy-har guy-har merged commit 3e66c7b into master Apr 9, 2024
39 of 45 checks passed
@guy-har guy-har deleted the fix/sts-subject branch April 9, 2024 14:20
emulatorchen pushed a commit to emulatorchen/lakeFS that referenced this pull request May 27, 2024
@guy-har
Use username as subject (treeverse#7637)
7e802f7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jonathan-Rosenberg Jonathan-Rosenberg Jonathan-Rosenberg approved these changes

@idanovo idanovo idanovo approved these changes

Assignees
No one assigned
Labels
include-changelog PR description should be included in next release changelog minor-change Used for PRs that don't require issue attached
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@guy-har @arielshaqed @Jonathan-Rosenberg @idanovo