Defining interfaces #3
Conversation
rayluo
force-pushed
the
defining-interfaces
branch
3 times, most recently
from
d5f4b2a to
b6465ff
Compare
| (Under the hood, we simply merge scope and additional_scope before | ||
| sending them on the wire.) | ||
| """ | ||
| a = Authority(self.authority, policy=policy) # TODO |
There was a problem hiding this comment.
- Remove the policy from Authority constructor.
- When writing a comment with "TODO", please also provide a summary about what needs to be done later, before you forget them.
| def get_authorization_request_url( | ||
| self, | ||
| scope, | ||
| # additional_scope=None, |
There was a problem hiding this comment.
We choose to provide both scope and optional additional_scope as a way to remind the developers the conceptual difference. In practice, the developer can still choose to use scope only, if they prefer.
| # before sending them on the wire. So there is no practical | ||
| # difference than removing this parameter and using scope only. | ||
| login_hint=None, | ||
| state=None, # TBD: It is not in MSAL-dotnet nor MSAL-Android, |
There was a problem hiding this comment.
Keep it here. All other MSAL libraries shall also add this parameter.
| self, | ||
| code, # TBD: | ||
| # .NET 's protected method defines 2 parameters: code, scope. | ||
| # .NET 's public method defines 2 parameters: scope, code. |
There was a problem hiding this comment.
Keep code as the first parameter. Remind MSAL-dotnet to do so too.
| code, # TBD: | ||
| # .NET 's protected method defines 2 parameters: code, scope. | ||
| # .NET 's public method defines 2 parameters: scope, code. | ||
| scope, # TBD: This could be optional. Shall it? See the document below. |
There was a problem hiding this comment.
Keep it as a required parameter
| # .NET 's public method defines 2 parameters: scope, code. | ||
| scope, # TBD: This could be optional. Shall it? See the document below. | ||
| redirect_uri=None, | ||
| # TBD: It is not in MSAL-dotnet. Do we need it? OAuth2 RFC says: |
There was a problem hiding this comment.
Remind dotnet implementation to add an overload method for redirect_uri too.
| # If absent, STS will give you a token associated to ONE of the scope | ||
| # sent in the authorization request. So only omit this when you are | ||
| # working with only one scope. | ||
| scope = scope or ["openid", "email", "profile", "offline_access"] # TBD |
There was a problem hiding this comment.
Only when the developer pass a single "client_id" as scope, then we change it into the predefined 4.
"client_id" -> ["openid", "email", "profile", "offline_access"]
There was a problem hiding this comment.
Addressed in this commit
| ): | ||
| # It will handle the TWO round trips of Authorization Code Grant flow. | ||
| raise NotImplemented() | ||
|
|
There was a problem hiding this comment.
Incline to add 2 more methods for device code grant here, rather than creating a dedicated sub class DeviceCodeClient.
rayluo
force-pushed
the
defining-interfaces
branch
from
b6465ff to
7ead4a3
Compare
This PR is created to facilitate the discussion on current API interface. The functional implementation is not completed and not intended to be merged as-is.