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

Refactor Acquire Token APIs Internally #792

Merged
merged 22 commits into from
Jul 22, 2019
Merged

Conversation

pkanher617
Copy link
Contributor

@pkanher617 pkanher617 commented Jun 26, 2019

This PR is to refactor our code for interactive acquire token calls, which will avoid repetition. There are some considerations to make here, including:

  • allowing request.account in login calls
  • removal of silentLogin
  • remove requirement to call login before acquireToken
  • acquireToken and loginInProgress call changes

The above changes can come in separate PRs that will be released in a major version, since these will be breaking changes.

Here is a summary of changes in this PR:

  • Refactored all interactive calls to a single function
  • fixed bug when sending clientId as only scope (?)
  • Renamed isCallback function to isStsResponse
  • Added new success and error handlers which will handle all interactive errors (will remove the old success handlers for redirect)
  • promptUser function renamed to navigateWindow, now takes a popup window, will navigate popups when given a window object
  • Moved around functions

@pkanher617 pkanher617 self-assigned this Jun 26, 2019
@DarylThayil
Copy link
Contributor

what are the breaking changes?

@DarylThayil
Copy link
Contributor

Might be good to schedule some time to go over this as a team, a lot of code changed and hard to ngrok it all

@pkanher617
Copy link
Contributor Author

what are the breaking changes?

There aren't any breaking changes yet, I should have edited the description. The considerations I have listed would be breaking changes depending on what we decide.

Might be good to schedule some time to go over this as a team, a lot of code changed and hard to ngrok it all

I will throw something on the calendar.

lib/msal-core/src/UserAgentApplication.ts Outdated Show resolved Hide resolved
lib/msal-core/src/UserAgentApplication.ts Outdated Show resolved Hide resolved
lib/msal-core/src/UserAgentApplication.ts Outdated Show resolved Hide resolved
@sameerag
Copy link
Member

sameerag commented Jul 2, 2019

I am not adding comments as we will go through this tomorrow in the review meetup. We need to follow this up with:

  1. The much-needed split of UserAgentApplication.ts file as sectioned already
  2. Break large functions in UAA to smaller ones (saveHashToken, processCallBack etc ...)

I know you are working on some PRs regarding this, I can help too, let me know and we can split up.

@pkanher617
Copy link
Contributor Author

isCallback is a breaking change

@sameerag
Copy link
Member

Me and @pkanher617 synced up offline and closed most review comments/follow-ups I had. We also created more internal PRs for future work to track some changes we wanted to get in later. I want to do a quick peek for a few more changes and will approve tomorrow.

Copy link
Contributor

@DarylThayil DarylThayil left a comment

Choose a reason for hiding this comment

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

lgtm, feel free to merge when you close with others on their open questions / get another approval

@pkanher617 pkanher617 merged commit 1b55af5 into dev Jul 22, 2019
@pkanher617 pkanher617 deleted the refactor_acquireToken branch July 30, 2019 20:01
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.

4 participants