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

refactor: Change response types of TOTP adapter to match existing adapters #8661

Merged
merged 7 commits into from
Jul 6, 2023

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Jun 25, 2023

Pull Request

Issue

The TOTP adapter has mixed return types, making it incompatible with strongly typed client SDKS.

There's also an additional issue with the TOTP adapter that is fixed in this PR as well.

Closes: #8660

Approach

Changes the response of enabled: true to status: "enabled"

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)

@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title refactor: update response of TOTP adapter refactor: Update response of TOTP adapter Jun 25, 2023
@parse-github-assistant
Copy link

Thanks for opening this pull request!

@dblythy
Copy link
Member Author

dblythy commented Jun 25, 2023

@cbaker6 let me know if this looks better

@codecov
Copy link

codecov bot commented Jun 25, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (a9d376b) 94.39% compared to head (4e9dd29) 94.39%.

❗ Current head 4e9dd29 differs from pull request most recent head 80ad75f. Consider uploading reports for the commit 80ad75f to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            alpha    #8661   +/-   ##
=======================================
  Coverage   94.39%   94.39%           
=======================================
  Files         185      185           
  Lines       14760    14761    +1     
=======================================
+ Hits        13932    13933    +1     
  Misses        828      828           
Impacted Files Coverage Δ
src/Adapters/Auth/mfa.js 81.73% <100.00%> (+0.16%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mtrezza mtrezza requested a review from a team June 25, 2023 07:32
@cbaker6
Copy link
Contributor

cbaker6 commented Jun 25, 2023

@dblythy the updates look good to me

@dblythy
Copy link
Member Author

dblythy commented Jun 26, 2023

I’ve just realised I’ve missed one thing

@dblythy
Copy link
Member Author

dblythy commented Jun 26, 2023

@cbaker6 I've also adapted the "additional auth data" when logging in,

Previous:

{
  username: 'username',
  password: 'password',
  authData: {
   mfa: totp.generate()
  },
}

New:

{
  username: 'username',
  password: 'password',
  authData: {
   mfa: {
     token: totp.generate()
   }
  },
}

Is this correct?

@mtrezza
Copy link
Member

mtrezza commented Jun 28, 2023

{
  username: 'username',
  password: 'password',
  authData: {
   mfa: {
     token: totp.generate()
   }
  },
}

would be a type structure of:

authData: object<string: object<string: string>>

It would be the same type structure that is in the REST API example:

{
  "authData": {
    "twitter": {
      "id": "12345678",
      "screen_name": "ParseIt",
      "consumer_key": "SaMpLeId3X7eLjjLgWEw",
      "consumer_secret": "SaMpLew55QbMR0vTdtOACfPXa5UdO2THX1JrxZ9s3c",
      "auth_token": "12345678-SaMpLeTuo3m2avZxh5cjJmIrAfx4ZYyamdofM7IjU",
      "auth_token_secret": "SaMpLeEb13SpRzQ4DAIzutEkCE2LBIm2ZQDsP3WUU"
    }
  }
}

So that should be good I'd say. Is any of this a breaking change, or is this PR only affecting code on the pre-release branches?

@dblythy
Copy link
Member Author

dblythy commented Jun 29, 2023

Only affects the alpha branch

@mtrezza
Copy link
Member

mtrezza commented Jun 29, 2023

So now both the request and response have a type structure of:

authData: object<string: object<string: string>>

Is that right?

@dblythy
Copy link
Member Author

dblythy commented Jun 30, 2023

Correct! Which should allow for easier integration in strongly typed SDKs, such as the SwiftSDK

@mtrezza
Copy link
Member

mtrezza commented Jul 3, 2023

Sounds good, I'll re-run the CI until it passes

@mtrezza
Copy link
Member

mtrezza commented Jul 3, 2023

Is this ready for merge?

@dblythy
Copy link
Member Author

dblythy commented Jul 3, 2023

@cbaker6 did you want to have one final review before we merge?

@mtrezza mtrezza added the block:beta Needs to be resolved before next release on beta branch; remove label afterwards label Jul 4, 2023
@parse-github-assistant
Copy link

The label block:beta cannot be used here.

@parse-github-assistant parse-github-assistant bot removed the block:beta Needs to be resolved before next release on beta branch; remove label afterwards label Jul 4, 2023
@mtrezza mtrezza changed the title refactor: Update response of TOTP adapter refactor: Change response types of TOTP adapter to match existing adapters Jul 6, 2023
@mtrezza mtrezza merged commit c9b5971 into parse-community:alpha Jul 6, 2023
@dblythy dblythy deleted the update-totp branch July 6, 2023 15:30
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.3.0-alpha.6

@parseplatformorg parseplatformorg added the state:released-alpha Released as alpha version label Jul 17, 2023
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.4.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Sep 16, 2023
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.4.0-alpha.1

@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.4.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Nov 16, 2023
@REPTILEHAUS
Copy link

REPTILEHAUS commented Dec 12, 2023

consumer_secret

@mtrezza I mentioned on the forum also but is it really fine to store any of the *_secret API keys on each user authData ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-alpha Released as alpha version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor TOTP adaper to return strings
5 participants