New version of otpauth_uri/4 with mandatory issuer

[samueltardieu]

The NimbleTOTP.otpauth_uri/2 function doesn't check that the label is well-formed or that the issuer present in the keyword matches the one which might be present in the label.

I noticed that when testing an URI generated by NimbleTOTP while trying to decode it with Rust's totp-rs crate.

[samueltardieu]

The new version still has two function, one which raises if the issuer/label is incorrect or if they are mismatched, and one which returns a more precise error.

I can:

• keep it as-is, and have two functions with different names in order to not break the API
• or break the API and rename the old one to otpauth_uri! and use otpauth_uri for the non-raising function
• or only keep the existing function enhanced with raising

Tell me what you prefer.

[josevalim]

I was thinking about this implementation:

  def otpauth_uri(label, secret, uri_params \\ []) when is_binary(label) and is_binary(secret) do
    issuer = uri_params[:issuer]

    cond
      is_nil(issuer) -> :ok
      String.contains?(issuer, ":") -> raise ArgumentError, ":issuer cannot contain \":\" in it, got: #{inspect(issuer)}"
      String.starts_with?(label, issuer <> ":") -> raise ArgumentError, "label should start with :issuer followed by colon, got: #{inspect(label)}"
      true -> :ok
    end

    key = Base.encode32(secret, padding: false)
    params = [{:secret, key} | uri_params]
    query = URI.encode_query(params, :rfc3986)
    "otpauth://totp/#{URI.encode(label)}?#{query}"
  end

The rationale to raise is that previous implementations that have any of those two properties are, at the moment, broken. We should not need a new function. What do you think?

[samueltardieu]

I am not sure I understand your proposal here: in the third cond clause, assuming there is a ! at the front, you want to force users to put the issuer in the label? Wouldn't that be an API change?

Another simple solution would be to have another function to let the user give the label and issuer separately in otpauth_uri/4 (while checking that they contain no colon and returning a proper error) and deprecate the current function. The deprecation warning would not break existing code but would encourage users to use the new function.

[josevalim]

Good points. ❤️ What about adding a new version like this:

  def otpauth_uri(issuer, account, secret, uri_params)
        when is_binary(issuer) and is_binary(account) and is_binary(secret) and is_list(uri_params) do
    issuer =~ ":" && raise ArgumentError, "issuer cannot have :"
    account =~ ":" && raise ArgumentError, "account cannot have :"
    key = Base.encode32(secret, padding: false)
    params = [issuer: issuer, secret: key] ++ uri_params
    query = URI.encode_query(params, :rfc3986)
    "otpauth://totp/#{URI.encode(issuer)}:URI.encode(account)?#{query}"
  end

And then we just deprecate the old one asking folks to call the new one?

The thing is that I am not sure I am convinced about ok/error. Because it seems they are all errors by construction, not something inherent to the function. If you are passing an invalid argument, it indicates something you could have pruned before hand.

[samueltardieu]

Agreed, I just came to the same conclusion that it is the user's fault to call the function with wrong parameters and raising is fine as it should be considered a programming error.

I like the new version, it ensures the URI is correct and respects the advices. The only change I would make would be to use the Keyword module to ensure that existing secret and issuer are replaced in the keyword list to ensure the otpauth URI correctness. Something like params = uri_params |> Keyword.put(:issuer, issuer) |> Keyword.put(:secret, key)

[josevalim]

That works for me! Would you like to update/send a PR with tests?

We can also add this special clause to the existing function:

def otpauth_uri(issuer, account, secret) when is_binary(issuer) and is_binary(account) and is_binary(secret) do
  otpauth_uri(issuer, account, secret, [])
end

So we make the fourth argument of the new one optional.

[samueltardieu]

One of the failure is related to Erlang installation, the other one complains about formatting but the PR is formatted (mix format on Elixir 1.18.4, and Elixir 1.19 gives the same result).

[samueltardieu]

One of the failure is related to Erlang installation, the other one complains about formatting but the PR is formatted (mix format on Elixir 1.18.4, and Elixir 1.19 gives the same result).

Oh, I didn't notice your new commit, it has two extra spaces on an empty comment line, hence the formatting failure.

[josevalim]

Thanks, I will update CI in a later commit! :)

[josevalim]

💚 💙 💜 💛 ❤️