Relax authority check

[abhidnya13]

This relaxes the authority url check to allow customers to add slashes after the tenant.
It then normalizes the url by removing these extra slashes.
Solves #156

[henrik-me]

lgtm

[stcheng]

thanks for fixing this.
looking forward to having it merged ASAP

[rayluo]

Thanks for your effort on implementation AND test cases! A couple minor questions below.

[rayluo]

The next line of normalization is needed for sure, but this elif ... clause seems not really necessary, does it? In such case, can we omit this elif ... line to emphasize the fact that there wouldn't be a third implicit code branch else ... scenario to be considered/reviewed here?

[abhidnya13]

There can be a case when the length is 0, in which case we would want to handle it differently and not normalize it. So I think the elif ... should be there.

[rayluo]

We discussed offline and clarified understandings. Since the rstrip(...) can handle case when length is 0 or more, the code could be written this conceptually different way:

        if len(path_parts) > 1:
            raise ValueError("The authority url must be of the format https://login.microsoftonline.com/your_tenant")
        self._url = urlparse(self._url.geturl().rstrip('/'))

yet the actual behavior would be the same.

[rayluo]

Out of curiosity, do we purposely use double slash // here (and the test case above)? It is rare to see // because only the backslash \\ would need to be doubled in a string literal.

To be clear, I know the implementation in this PR happens to be able to handle multiple trailing slashes, which is a sweet side effect to have. But from the intention perspective, the expectation was to handle (at least) one trailing slash, and if we wrote our test cases with //, it does not explicitly indicate that we can definitely handle /.

PS: @stcheng also had confusion on //, based on the github notification emails.

[abhidnya13]

The number of slashes here actually don't matter because any number of slashes should be stripped. It just makes sure any number of slashes after the authority url are stripped before going further.

[abhidnya13]

I can change it real quick 👍

[henrik-me]

why not have test for both?

---

In reply to: 194158095 [](ancestors = 194158095)

[rayluo]

@henrik-me We could test both if we really want to. Personally I consider a test case for // (or ///, ////, ..., for that matter) is optional, because: (1) we won't be able to cover all possible scenario anyway, (2) multiple adjacent slashes is not a valid url path in the first place, it seems not in our interest to have a formal extra test case here to demonstrate our SDK can handle it.

[abhidnya13]

What if we add a comment in the existing one which explains that it is designed to handle one or more slashes?

[henrik-me]

tests should cover both good and bad scenarios. it's not all but ensuring we cover test cases ensuring we do not break the behavior we introduced (namely stripping of all / and ensuring there is always only 1 when we create the url. (I suppose I do not understand based on your comment Ray why we would even have changed the test in the first place)

[henrik-me]

Aside: I already gave sign-off.

[rayluo]

LGTM