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

feat: remove API secret in favor of explicitly passing user HMAC #31

Merged
merged 8 commits into from
Apr 11, 2024
Merged

feat: remove API secret in favor of explicitly passing user HMAC #31

merged 8 commits into from
Apr 11, 2024

Conversation

stigi
Copy link
Contributor

@stigi stigi commented Apr 10, 2024

Note

This PR supersedes #32

In this PR

This changes the way the Swift SDK handles user HMACs. Previously it was expected to pass the API secret so the client side SDK would calculate the HMAC. This is a bad practice and discouraged.

With this change the SDK only allows for a enableHMAC toggle, and an optional hmac parameter to the connectUser call.

Test Plan

I've tested the change in the Example app by using the API key from a project with Security enabled.
I've also extended the Example app so that one can provide an hmac when changing to a different user.

I could confirm that notifications did load when providing a correct hmac, while they returned console errors when no hmac was provided (added line breaks for readability):

{"errors":[{
  "code":"hmac_header_not_provided",
  "message":"HMAC header not provided",
  "suggestion":"Please provide the 'X-MAGICBELL-USER-HMAC' header containing the HMAC of the user's email. The HMAC is generated using the user's email/external_id and your MagicBell project's API secret.",
  "help_link":"https://www.magicbell.com/docs/hmac-authentication"}]}

When providing an invalid hmac the following error was provided (added line breaks for readability):

 {"errors":[{
  "code":"hmac_header_invalid",
  "message":"HMAC header should be Base64 encoded",
  "suggestion":"Please provide a valid Base64 encoded value for the 'X-MAGICBELL-USER-HMAC' header",
  "help_link":"https://www.magicbell.com/docs/hmac-authentication"}]}

Open questions:

  1. Those errors are only bubble up in the console and the SDK seems to not provide a way yet to handle these errors in the client app
  2. Is the enableHMAC flag on MagicBellClient actually needed? It could be deferred by whether an hmac is passed into connectUser. Also the truth whether hmac is enabled resides on the Magicbell project configuration on the server. Resolved in ba8e20d
  3. The documentation on https://www.magicbell.com/docs/libraries/ios needs to be updated according to the changes in here.

@stigi stigi changed the title [WIP] Removes API secret in favor of explicitly passing user HMAC Removes API secret in favor of explicitly passing user HMAC Apr 10, 2024
@stigi stigi marked this pull request as ready for review April 10, 2024 19:56
@smeijer smeijer changed the title Removes API secret in favor of explicitly passing user HMAC feat: remove API secret in favor of explicitly passing user HMAC Apr 11, 2024
@stigi
Copy link
Contributor Author

stigi commented Apr 11, 2024

Good that we have tests running again now. Looks like they just need to be updated.

@stigi stigi merged commit c46bedf into magicbell:main Apr 11, 2024
1 check passed
@stigi stigi deleted the ullrich/remove-api-secret branch April 11, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants