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

[SDK-2750] Expose mfa_token from the mfa_required error when getting new tokens #789

Merged
merged 6 commits into from
Sep 8, 2021

Conversation

frederikprijck
Copy link
Member

When mfa is enabled, the /token endpoint can return a mfa_required error. In the case that it does, it also included a mfa_token property, which we are currently ignoring.

This PR introduces a MfaRequiredError class that is used to unwrap the above information in case of a mfa_required error, exposing the mfa_token to the users.

@frederikprijck frederikprijck requested a review from a team as a code owner September 6, 2021 13:22
@@ -133,16 +133,20 @@ export async function getJSON<T>(
}

const {
json: { error, error_description, ...success },
json: { error, error_description, ...data },
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed this to data, because it contains the mfa_token and it was a bit weird to use success in case of an error.

@frederikprijck frederikprijck added CH: Added PR is adding feature or functionality review:small Small review labels Sep 6, 2021
@@ -30,7 +30,7 @@ export class AuthenticationError extends GenericError {
public state: string,
public appState: any = null
) {
super(error, error_description);
super(error, error_description) /* istanbul ignore next */;
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a few istanbul ignores, see gotwarlost/istanbul#690

lbalmaceda
lbalmaceda previously approved these changes Sep 7, 2021
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@stevehobbsdev stevehobbsdev left a comment

Choose a reason for hiding this comment

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

LG, just left a couple things around exports

src/errors.ts Show resolved Hide resolved
@frederikprijck frederikprijck merged commit 6806c83 into master Sep 8, 2021
@frederikprijck frederikprijck deleted the frederik/sdk-2750 branch September 8, 2021 20:55
@stevehobbsdev stevehobbsdev mentioned this pull request Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Added PR is adding feature or functionality review:small Small review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants