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

MSAL 2.x beta release #1421

Merged
merged 24 commits into from Mar 31, 2020
Merged

MSAL 2.x beta release #1421

merged 24 commits into from Mar 31, 2020

Conversation

pkanher617
Copy link
Contributor

@pkanher617 pkanher617 commented Mar 25, 2020

PR preparing for the beta release of MSAL 2.x

@pkanher617 pkanher617 changed the base branch from dev to e2e-test-framework March 25, 2020 21:16
@pkanher617 pkanher617 self-assigned this Mar 25, 2020
@coveralls
Copy link

coveralls commented Mar 25, 2020

Coverage Status

Coverage remained the same at 77.04% when pulling 5b57594 on msal2-beta0-release into 857ff1f on dev.

Copy link
Contributor

@jmckennon jmckennon 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, but do we want all of the .md files to be all lowercase ex: acquiretoken.md vs AcquireToken.md vs acquire-token.md, etc.

};
```

| Parameter | Description | Format |
Copy link
Contributor

Choose a reason for hiding this comment

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

one other thing, do we care about normalizing the readability of these markdown tables? like having the spaces between | always be the same size, etc., so it'd look more like:

| `uniqueId` | `oid` or `sub` claim from ID token. | string |
| `tenantId` | `tid` claim from ID token.          | string |

basically lined up with the longest row in each column

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do that incrementally. I wrote these with the intent of viewing them in browser, but if it is easier to format these raw docs then I am happy to put some time into it. There may be libs or plugins that do it for us as well.

lib/msal-browser/FAQ.md Show resolved Hide resolved
lib/msal-browser/README.md Outdated Show resolved Hide resolved
lib/msal-browser/README.md Outdated Show resolved Hide resolved
lib/msal-browser/README.md Outdated Show resolved Hide resolved
lib/msal-browser/README.md Show resolved Hide resolved
lib/msal-browser/docs/configuration.md Outdated Show resolved Hide resolved

## Configuration Options

### Auth Config Options
Copy link
Contributor

Choose a reason for hiding this comment

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

I would argue this section should be deleted and replaced with a link to the reference docs. We know we don't want to be maintaining this in multiple places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't get the reference docs working so I wrote this for now. We can remove this once the I get the publishing for those docs working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update this once reference docs are updated to be more detailed. for now I will keep this here for customers to use.

lib/msal-browser/docs/logging.md Outdated Show resolved Hide resolved

| Option | Description | Format |
| ------ | ----------- | ------ |
| `scopes` | See [below](#scopes). | String array |
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about reference docs, why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update this once reference docs are updated to be more detailed. for now I will keep this here for customers to use.

lib/msal-browser/docs/v1migration.md Outdated Show resolved Hide resolved
@jasonnutter
Copy link
Contributor

Looks good, but do we want all of the .md files to be all lowercase ex: acquiretoken.md vs AcquireToken.md vs acquire-token.md, etc.

Agreed, would prefer dash-cased titles.

@pkanher617 pkanher617 changed the base branch from e2e-test-framework to dev March 30, 2020 22:04
Copy link
Contributor

@jmckennon jmckennon 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


# Will MSAL 2.x support B2C?

We are currently working with the B2C service team to allow for authorization code flow to work in the browser with B2C tenants. We hope to have this available shortly after release.
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have the need for any other FAQs except work in progress tickets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add to it as we continue, I have only added questions I have received from the alpha.

3. Click next and create a name for the shortcut (i.e. Chrome-no-cors)
- For other machines, you can follow the steps [here](https://alfilatov.com/posts/run-chrome-without-cors/) to figure out how to run Chrome withput CORS enabled for your OS.
- Once you have registered your application, you will need to do two things in order to ensure `@azure/msal-browser` will successfully retrieve tokens:
1. Change your redirect URI type to enable CORS. You can do this by going to the manifest editor for your app registration in the portal, finding the `replyUrlsWithType` section and changing the type of your redirect URI to `SPA`. This may remove the affected redirect URIs from the Web platform Authentication tab - that's OK! We are working on getting UI set up for this.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think spa may be lowercase? I'm not sure if it matters but can you double check on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed it to uppercase and the manifest changes it to "Spa" so I think it is case insensitive


**IMPORTANT:** Please be aware that this is not a production ready library. You are required to use a browser that disables CORS checks. The server will be making changes that will allow CORS requests and remove the requirement for client secret for applications which are registered using a `SPA` type redirect.
**IMPORTANT:** Please be aware that this is not a production ready library. We are making changes in the [Azure portal](https://azure.microsoft.com/en-us/features/azure-portal/) to ensure we can deliver a polished end-to-end experience.
Copy link
Member

Choose a reason for hiding this comment

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

The portal changes are still to come?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes they are not in yet.

},
system: {
loggerOptions: {
loggerCallback: (level: LogLevel, message: string, containsPii: boolean): void => {
Copy link
Member

Choose a reason for hiding this comment

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

Haven't we decided to have the option for the library to work without the logger call back with "debug"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove this when the logger changes go in, for now I am going to provide a callback for logs.

| ------ | ----------- | ------ |
| `forceRefresh` | If true, `acquireTokenSilent` will not returned the cached tokens and will use the refresh token to retrieve a new set of tokens. | boolean |

### Scopes
Copy link
Member

Choose a reason for hiding this comment

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

not needed right now, but @jasonnutter we can review the scopes info here from the POV of redirecting all scopes usage related questions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1, it would be good to start having repo docs for all usage related questions.

Copy link
Member

@sameerag sameerag left a comment

Choose a reason for hiding this comment

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

Approved, Good work @pkanher617. I will continue to check the content as we go forward and give feedback, we can retain the common sections for msal-common as independent docs referred as needed.

@pkanher617 pkanher617 merged commit c997f66 into dev Mar 31, 2020
@pkanher617 pkanher617 deleted the msal2-beta0-release branch June 8, 2020 16:25
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.

None yet

7 participants