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

WebUI MFA types refactor #49678

Merged
merged 10 commits into from
Dec 9, 2024
Merged

WebUI MFA types refactor #49678

merged 10 commits into from
Dec 9, 2024

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Dec 3, 2024 •
edited
Loading

Changes:

  • Move, deduplicate, and add MFA types to web/packages/teleport/src/services/mfa
    • Also rename some types/methods and improve type assertions.
  • Refactor MFA options to be derived from MFA challenges rather than listed devices, which is more reliable (backend logic) and removes the need for a lot of extra logic we were doing based off of auth2faType

Prerequisite for SSO MFA changes (TODO).

TODO: Follow up PRs to remove remaining uses of createMfaOptions.

@Joerger Joerger requested a review from avatus December 3, 2024 02:39
@github-actions github-actions bot requested review from kiosion and ryanclark December 3, 2024 02:40
This was referenced Dec 3, 2024
Merged
Open
ryanclark
ryanclark reviewed Dec 3, 2024
View reviewed changes
web/packages/shared/utils/createMfaOptions.ts Outdated
};
}

return opt as MfaOption;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't opt already MfaOption here?

Copy link
Contributor Author

@Joerger Joerger Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's of type services/mfa/MfaOption but this method needs to return utils/MfaOption. I'm not sure how to how to make the former a type alias of the latter since they have the same name, so I just did a type cast. Is there another way to make this work?

Copy link
Contributor

Choose a reason for hiding this comment

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

would using satisfies MfaOption work here to coerce it w/out asserting?

Copy link
Contributor Author

@Joerger Joerger Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

would using satisfies MfaOption work here to coerce it w/out asserting?

Maybe? But I'm not sure how to import services/mfa/MfaOption since the name overlaps.

Copy link
Contributor

Choose a reason for hiding this comment

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

import { MfaOption as SomeOtherMfaOption }

web/packages/shared/utils/createMfaOptions.ts Outdated Show resolved Hide resolved
web/packages/teleport/src/services/mfa/mfaOptions.ts Outdated Show resolved Hide resolved
@Joerger Joerger requested a review from ryanclark December 3, 2024 18:51
@Joerger Joerger added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Dec 3, 2024
@Joerger Joerger force-pushed the joerger/unify-mfa-types branch from a190b68 to 7ae2721 Compare December 3, 2024 18:55
@Joerger Joerger force-pushed the joerger/unify-mfa-types branch from 7ae2721 to e0ee68e Compare December 3, 2024 20:33
@Joerger Joerger force-pushed the joerger/unify-mfa-types branch from e0ee68e to 9755a70 Compare December 3, 2024 23:31
@Joerger Joerger mentioned this pull request Dec 4, 2024
Open
@Joerger Joerger force-pushed the joerger/unify-mfa-types branch from 2704ebc to 916e0fe Compare December 4, 2024 20:44
@rosstimothy
Copy link
Contributor

friendly ping @ryanclark @kiosion

ryanclark
ryanclark approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@ryanclark ryanclark left a comment

Choose a reason for hiding this comment

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

Sorry was OOO

@Joerger Joerger added this pull request to the merge queue Dec 9, 2024
Merged via the queue into master with commit 21cdf9e Dec 9, 2024
39 checks passed
@Joerger Joerger deleted the joerger/unify-mfa-types branch December 9, 2024 21:08
@public-teleport-github-review-bot
Copy link

@Joerger See the table below for backport results.

Branch Result
branch/v15 Failed
branch/v16 Failed
branch/v17 Create PR

@Joerger Joerger mentioned this pull request Dec 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanclark ryanclark ryanclark approved these changes

@avatus avatus avatus approved these changes

@kiosion kiosion Awaiting requested review from kiosion

Assignees
No one assigned
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/md ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Joerger @rosstimothy @avatus @ryanclark @kiosion