firebase-ios-sdk icon indicating copy to clipboard operation
firebase-ios-sdk copied to clipboard

Incorrect comment in code.

Open mortenbekditlevsen opened this issue 3 years ago • 3 comments

The comment on the referred line specifies that the order of supportedAuthDomains matters. But the outer loop is the authorized domains from the web request, so actually the first match in the authorized domains list 'wins'. The inner and outer loops should be switched for the comment to be correct.

https://github.com/firebase/firebase-ios-sdk/blob/31307cdf7e2e7f0cf931ccf30f54fb409cad037d/FirebaseAuth/Sources/Utilities/FIRAuthWebUtils.m#L107

mortenbekditlevsen avatar Apr 28 '21 07:04 mortenbekditlevsen

I found a few problems with this issue:

  • I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
  • This issue does not seem to follow the issue template. Make sure you provide all the required information.

google-oss-bot avatar Apr 28 '21 07:04 google-oss-bot

Just to make my initial comment a bit clearer:

Currently, the supportedAuthDomains order does not matter (as opposed to what's stated in the comment) - the returned domain will be determined by the order of response.authorizedDomains.

If response.authorizedDomains contains ["my.web.app", "my.firebaseapp.com"], then we would first look to see if my.web.app matched any of ["firebaseapp.com", "web.app"], which it does and then "my.web.app" will be used.

If the response order was different: ["my.firebaseapp.com", "my.web.app"], then the first match would be for my.firebaseapp.com.

And thus, contrary to the comment, it's not the order of supportedAuthDomains that matters, but the order of response.authorizedDomains that matters. And the latter is not controlled by the library, so that seems like an error - if indeed the order of supportedAuthDomains matters.

mortenbekditlevsen avatar Aug 31 '21 10:08 mortenbekditlevsen

@rosalyntan Should the comment be fixed or the code updated to match the comment?

cc: @renkelvin

paulb777 avatar Jun 30 '22 23:06 paulb777

I think we should update the code to match the comment.

renkelvin avatar Jun 12 '23 17:06 renkelvin