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

Add auth factor methods #217

Merged
merged 5 commits into from
Nov 20, 2023
Merged

Add auth factor methods #217

merged 5 commits into from
Nov 20, 2023

Conversation

blairworkos
Copy link
Contributor

Description

Adds methods to enroll an authentication factor and list the authentication factors for a user.

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

@blairworkos blairworkos self-assigned this Nov 20, 2023
@blairworkos blairworkos requested a review from a team as a code owner November 20, 2023 18:55
@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (fb0e087) 87.72% compared to head (3664c30) 87.80%.

Files Patch % Lines
workos/user_management.py 95.65% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
+ Coverage   87.72%   87.80%   +0.08%     
==========================================
  Files          32       32              
  Lines        1157     1173      +16     
==========================================
+ Hits         1015     1030      +15     
- Misses        142      143       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

workos/resources/mfa.py Outdated Show resolved Hide resolved
workos/resources/mfa.py Outdated Show resolved Hide resolved

def list_auth_factors(
self,
user,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we tend to use params like user or user_id in the python SDK more? what about within the user management module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be more like user, connection, organization, etc. It definitely seems like doing the _id would be better, but that is the convention we're currently using elsewhere:

user (str) - User unique identifier

def get_connection(self, connection):

def get_organization(self, organization):

Copy link
Contributor

Choose a reason for hiding this comment

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

and we use organization_id here... but that's a list endpoint

organization_id=None,

Copy link
Contributor

Choose a reason for hiding this comment

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

blegh! lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yikes 🙈 I'm ok with starting to be more opinionated and go the *_id route

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah how about within the UM module we go w/ *_id?

fwiw we were also inconsistent at the API layer but for UM have made all params *_id (and that'll be the plan for new APIs going forward)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool cool - I'll make those changes throughout the module now 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made those changes throughout the UM module, just to remind myself for later changes

Copy link
Contributor

@amygdalama amygdalama left a comment

Choose a reason for hiding this comment

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

ty!

@blairworkos blairworkos merged commit ae25324 into main Nov 20, 2023
6 checks passed
@blairworkos blairworkos deleted the add-auth-factor-methods branch November 20, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants