Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored and fixed device code sample #1

Merged
merged 13 commits into from
Jun 15, 2020
Merged

Conversation

hchittanuru3
Copy link
Contributor

  • Refactored imports to make it work locally
  • Added a device code sample in src/examples/devicecodeflow/main.go
  • Fixed DeviceCodeRequest and Token Response to handle authorization_pending and slow_down errors

@henrik-me henrik-me requested a review from SomkaPe May 29, 2020 22:32
@henrik-me
Copy link
Contributor

consider sharing code between the samples and refactor usernamepwd flow to match what you added for device code... or build one sample that can do both and ask for the user for a key to pick auth method

@henrik-me
Copy link
Contributor

Do you mind adding a bit more to the readme file on how to run the samples?

Copy link
Contributor

@henrik-me henrik-me left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@hchittanuru3
Copy link
Contributor Author

hchittanuru3 commented Jun 1, 2020

consider sharing code between the samples and refactor usernamepwd flow to match what you added for device code... or build one sample that can do both and ask for the user for a key to pick auth method

Added in this commit.

@hchittanuru3
Copy link
Contributor Author

hchittanuru3 commented Jun 1, 2020

Do you mind adding a bit more to the readme file on how to run the samples?

Added this in this commit.

Copy link
Contributor

@sangonzal sangonzal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great. Thank for cleaning up.

Also one more thing that we need to have a full device code implementation:

  • A mechanism so that callers can cancel polling. While the polling is happening, the caller should be able to set cancel to true and that would cause the MSAL to break out of the polling loop.

src/internal/requests/DeviceCodeRequest.go Outdated Show resolved Hide resolved
@hchittanuru3
Copy link
Contributor Author

In this new commit, the following changes have been made:

  1. PublicClientApplication now has a AcquireDeviceCode function, so the device code result values can be returned to the user, instead of logged to the terminal.
  2. The flow has slightly changes, so users need to call AcquireDeviceCode and use the DeviceCodeRequest value as a parameter for AcquireTokenByDeviceCode.
  3. The DeviceCodeRequest now stores a DeviceCodeResult as well as a cancel flag.
  4. The DeviceCodeSample has both examples of device code flow with and without cancellation.

Copy link
Contributor

@sangonzal sangonzal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress. A couple of questions.

@@ -22,3 +24,9 @@ func (p *AcquireTokenDeviceCodeParameters) augmentAuthenticationParameters(authP
p.commonParameters.augmentAuthenticationParameters(authParams)
authParams.SetAuthorizationType(msalbase.AuthorizationTypeDeviceCode)
}

func (p *AcquireTokenDeviceCodeParameters) InternalCallback(dcr *msalbase.DeviceCodeResult) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an internal callback? Can we just call deviceCodeCallback directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to cast it to the interface IDeviceCodeResult. The internal library returns a DeviceCodeResult, and to make it accessible to the user, we utilize IDeviceCodeResult.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the interface in the method signatures internally as well? We should try to use the interface wherever possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interface is not accessible to the internal package. The DeviceCodeRequest uses this callback and passes in a DeviceCodeResult, which this InternalCallback casts to the interface type IDeviceCodeResult.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the interface accessible to the internal package?

src/PublicClientApplication.go Outdated Show resolved Hide resolved
src/PublicClientApplication.go Outdated Show resolved Hide resolved
src/examples/DeviceCodeFlowSample.go Outdated Show resolved Hide resolved
src/internal/msalbase/DeviceCodeResult.go Show resolved Hide resolved
src/internal/requests/DeviceCodeRequest.go Outdated Show resolved Hide resolved
src/internal/requests/DeviceCodeRequest.go Outdated Show resolved Hide resolved
src/internal/msalbase/DeviceCodeResult.go Outdated Show resolved Hide resolved
src/PublicClientApplication.go Show resolved Hide resolved

req.authParameters.SetAuthorityEndpoints(endpoints)
deviceCodeResult, err := req.webRequestManager.GetDeviceCodeResult(req.authParameters)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update GetDeviceCodeResult to return IDeviceCodeResult? In line 50, req.deviceCodeCallback() can then be updated to be the callback set by the developer, instead of the internal callback?

@@ -22,3 +24,9 @@ func (p *AcquireTokenDeviceCodeParameters) augmentAuthenticationParameters(authP
p.commonParameters.augmentAuthenticationParameters(authParams)
authParams.SetAuthorizationType(msalbase.AuthorizationTypeDeviceCode)
}

func (p *AcquireTokenDeviceCodeParameters) InternalCallback(dcr *msalbase.DeviceCodeResult) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the interface accessible to the internal package?

@hchittanuru3 hchittanuru3 linked an issue Jun 9, 2020 that may be closed by this pull request
@hchittanuru3 hchittanuru3 requested a review from sangonzal June 10, 2020 18:56
Copy link
Contributor

@sangonzal sangonzal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. :shipit:

@hchittanuru3 hchittanuru3 merged commit ee1f38a into dev Jun 15, 2020
@hchittanuru3 hchittanuru3 deleted the device-code-sample branch June 15, 2020 17:17
handsomejack-42 added a commit that referenced this pull request Apr 9, 2024
…t readability

Previously, the test results were printed as #0 #1 #2, which made it
hard to navigate the failed test cases. Now, the test cases
are annotated by their respective names. The names also make it
more clear what is being unit-tested.
handsomejack-42 added a commit that referenced this pull request May 9, 2024
…t readability

Previously, the test results were printed as #0 #1 #2, which made it
hard to navigate the failed test cases. Now, the test cases
are annotated by their respective names. The names also make it
more clear what is being unit-tested.
handsomejack-42 added a commit that referenced this pull request Nov 4, 2024
…t readability

Previously, the test results were printed as #0 #1 #2, which made it
hard to navigate the failed test cases. Now, the test cases
are annotated by their respective names. The names also make it
more clear what is being unit-tested.
bgavrilMS pushed a commit that referenced this pull request Nov 5, 2024
…t readability

Previously, the test results were printed as #0 #1 #2, which made it
hard to navigate the failed test cases. Now, the test cases
are annotated by their respective names. The names also make it
more clear what is being unit-tested.
4gust pushed a commit that referenced this pull request Nov 8, 2024
…t readability

Previously, the test results were printed as #0 #1 #2, which made it
hard to navigate the failed test cases. Now, the test cases
are annotated by their respective names. The names also make it
more clear what is being unit-tested.
4gust pushed a commit that referenced this pull request Nov 11, 2024
…t readability

Previously, the test results were printed as #0 #1 #2, which made it
hard to navigate the failed test cases. Now, the test cases
are annotated by their respective names. The names also make it
more clear what is being unit-tested.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get Device Code Flow to work (Public Client App)
3 participants