firebase-ios-sdk
firebase-ios-sdk copied to clipboard
Incorrect comment in code.
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
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.
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.
@rosalyntan Should the comment be fixed or the code updated to match the comment?
cc: @renkelvin
I think we should update the code to match the comment.