move reserved scopes to a module level constant

[nwalsh1995]

We are wrapping this library and are allowing users to pass in scopes. We need to make sure they don't pass in any scopes that are reserved so that they don't receive an error. However, the reserved_scope is closed by the function so we need to move it outside for visibility.

I would also like some clarity on why this check is being performed anyway. It seems unnecessary since later we union the two sets or just use the reserved set itself. I believe this function violates the principle of least astonishment and this check of reserved scopes should be removed completely.

[rayluo]

some clarity on why this check is being performed anyway. It seems unnecessary since later we union the two sets or just use the reserved set itself. I believe this function violates the principle of least astonishment and this check of reserved scopes should be removed completely.

That check was established, across all ADAL/MSAL libraries, before I joined this team. I believe it was about the API contract we wanted to promise. For example, since 5+ years ago, the ADAL/MSAL library purposely (or even proudly?) switched to NOT returning a refresh token (RT), even though ADAL/MSAL internally acquires RTs to properly function. Now, imagine an app developer calls our API with scope=... offline_access ... and our API accepts such input, but silently filters out the RT from the response, now the app dev would be forced into a debug session to figure out why the RT was missing. That would be a much bigger astonishment to warrant some 4-letter words. So, the team chose to be clear that this API won't return an RT, by rejecting those scopes input upfront. I suspect there might be a similar story for id token.

That being said, nowadays, that decision to hide RTs are not followed as strictly as before. Some of our ADAL/MSAL do return RT. So, we might actually revisit this sometime later. That was partially why, when I wrote the current implementation, I chose to make it as an internal implementation detail that could not be referenced by downstream callers. We do not think a downstream package would want to make a dependence on such a quirky behavior.

The way you proposed in this PR, has an implication. It would expose such concept as part of the API surface (albeit undocumented), and then you actually plan to depend on it. Should we actually remove such reserved_scopes behavior in the future, the repo maintainers would not necessarily remember why that module-level RESERVED_SCOPES was there. Then it will likely be deleted, and your package will be broken.

I would rather suggest you to:

1. leave it alone and let the MSAL ValueError bubble up. It contains a clear error message, which YOUR downstream app developer would only need to see and handle once.
2. or same as above, but then do a catch-and-absorb-and-throw-a-new-exception, to hide the exception stack so that your downstream consumers won't see anything from MSAL
3. or you just copy-and-paste that reserved_scopes definition in your wrapper package without creating a strong tie between the two. (Nope, we do not foresee we would ever modify it again. The next time we touch it would likely be its removal.)
4. or you simply make a dependency to the already-existing decorate_scope(scopes, "dummy_client_id") helper (which is conceptually still an internal helper in MSAL even though we did not give it a leading underscore 😛 but it is there anyway)

[nwalsh1995]

Hi @rayluo, thanks for the background on this. It is very enlightening to know the history of the library.

copy-and-paste that reserved_scopes definition in your wrapper package without creating a strong tie between the two.

I didn't want to do this because if you end up modifying reserved_scopes to include some other attribute then our library and MSAL is not compatible.

let the MSAL ValueError bubble up (it contains a clear error message, which YOUR downstream app developer would only need to see and handle once), or do a catch-and-rethrow (to hide the exception stack)

Users using our library should not know it uses MSAL underneath the hood since it is an implementation detail of how we are approaching SSO. Therefore, the burden is actually on our library implementation to hide the details of this. In our specific case it is important to know if they are passing anything in reserved_scopes so the library can handle it however it is necessary. It being closed off in the function means that we have no insight on which values are allowed / reserved, so the goal was to move them into something our library can import/use.

How about having _RESERVED_SCOPES marked with an underscore to signify that it is 'private'? That way you can make API changes to it without guaranteeing its consistency.

[nwalsh1995]

We will approach with the third point in your suggestion. Closing out.