Skip to content

Extend IsMFARequired check to admin actions#36785

Merged
Joerger merged 5 commits intomasterfrom
joerger/is-mfa-required-for-admin-actions
Jan 19, 2024
Merged

Extend IsMFARequired check to admin actions#36785
Joerger merged 5 commits intomasterfrom
joerger/is-mfa-required-for-admin-actions

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Jan 17, 2024

Adds an admin action target to IsMFARequired.

Also adds IsMFARequired field to web.createAuthenticateChallenge to mirror grpc CreateAuthenticateChallenge functionality and skip the extra roundtrip.

This will be used for admin action MFA inference and reuse - #35185.

@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Jan 17, 2024
@github-actions github-actions Bot requested a review from gzdunek January 17, 2024 05:00
@github-actions github-actions Bot requested a review from ryanclark January 17, 2024 05:00
@github-actions github-actions Bot added the ui label Jan 17, 2024
Comment thread web/packages/teleport/src/services/auth/auth.ts Outdated
Comment thread lib/web/mfa.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From what I see, we usually use camel case for json fields.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks pretty split actually. snake_case seems to be slightly more common in our code and is more conventional for json fields IIUC.

Comment thread web/packages/teleport/src/services/auth/auth.ts Outdated
Comment thread web/packages/teleport/src/services/user/user.ts Outdated
@Joerger Joerger force-pushed the joerger/is-mfa-required-for-admin-actions branch from c60f31f to b5e59d0 Compare January 17, 2024 20:31
@Joerger Joerger force-pushed the joerger/is-mfa-required-for-admin-actions branch from b5e59d0 to 79097b5 Compare January 17, 2024 21:17
@Joerger Joerger requested a review from gzdunek January 17, 2024 21:17
@Joerger Joerger force-pushed the joerger/is-mfa-required-for-admin-actions branch 2 times, most recently from 9a23446 to 85ce35a Compare January 18, 2024 19:36
@Joerger Joerger force-pushed the joerger/is-mfa-required-for-admin-actions branch from 85ce35a to bac2198 Compare January 18, 2024 20:57
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

With minor comments.

Comment thread web/packages/teleport/src/services/user/user.ts Outdated
Comment thread web/packages/teleport/src/services/auth/auth.ts Outdated
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from ryanclark January 19, 2024 12:13
@Joerger Joerger enabled auto-merge January 19, 2024 19:30
@Joerger Joerger added this pull request to the merge queue Jan 19, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jan 19, 2024
@Joerger Joerger added this pull request to the merge queue Jan 19, 2024
Merged via the queue into master with commit 248db09 Jan 19, 2024
@Joerger Joerger deleted the joerger/is-mfa-required-for-admin-actions branch January 19, 2024 20:46
@public-teleport-github-review-bot
Copy link
Copy Markdown

@Joerger See the table below for backport results.

Branch Result
branch/v15 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/md ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants