Skip to content
This repository was archived by the owner on May 17, 2024. It is now read-only.

Update 5.1 to follow basher and zero trust guidelines #196

Merged
merged 18 commits into from
Sep 7, 2022
Merged

Update 5.1 to follow basher and zero trust guidelines #196

merged 18 commits into from
Sep 7, 2022

Conversation

@derisen
Copy link
Contributor

@derisen derisen commented Aug 29, 2022 •
edited
Loading

Purpose

  • This updates Angular App Roles sample to follow basher and zero trust guidelines

Does this introduce a breaking change?

[ x ] Yes [ ] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ x ] Bugfix [ x ] Feature [ ] Code style update (formatting, local variables) [ ] Refactoring (no functional changes, no api changes) [ x ] Documentation content changes [ x ] Other... Please describe: dependency updates

How to Test

  • Get the code
git clone https://github.com/Azure-Samples/ms-identity-javascript-angular-tutorial.git cd ms-identity-javascript-angular-tutorial git checkout basher-5-1 cd 5-AccessControl/1-call-api-roles/SPA npm install cd ../API/TodoListAPI dotnet restore
  • Test the code
    • Run the sample end-to-end
    • Assign one user to admin role, and test access
    • Assign one user to user role, and test access
@derisen derisen linked an issue Aug 29, 2022 that may be closed by this pull request
When using the MSALGuard and RoleGuard on a route, the Role guard will allow access even if the user does not have the correct role #180
Closed
@derisen derisen changed the title Basher 5 1 Update 5.1 to follow basher and zero trust guidelines Aug 29, 2022
kalyankrishna1
kalyankrishna1 reviewed Aug 31, 2022
View reviewed changes
5-AccessControl/1-call-api-roles/README.md
1. Ensure that the **My APIs** tab is selected.
1. In the list of APIs, select the API `msal-angular-app`.
* Since this app signs-in users, we will now proceed to select **delegated permissions**, which is is requested by apps when signing-in users.
1. In the **Delegated permissions** section, select the **access_as_user** in the list. Use the search box if necessary.
Copy link
Contributor

@kalyankrishna1 kalyankrishna1 Aug 31, 2022

Choose a reason for hiding this comment

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

  1. In the Delegated permissions section, select the access_as_user in the list. Use the search box if necessary.

codegen is generating "1. In the Delegated permissions section, select the Access 'msal-angular-app' in the list. Use the search box if necessary."

could you look this in code gen for me please?
Copy link
Contributor Author

@derisen derisen Aug 31, 2022

Choose a reason for hiding this comment

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

will do -created an issue: https://github.com/Azure-Samples/GenerateScriptsForSample/issues/32
@kalyankrishna1
Copy link
Contributor

kalyankrishna1 commented Aug 31, 2022

npm start

npm restore

Refers to: 5-AccessControl/1-call-api-roles/README.md:232 in 88b0b70. [](commit_id = 88b0b70, deletion_comment = False)
kalyankrishna1
kalyankrishna1 reviewed Aug 31, 2022
View reviewed changes
salman90
salman90 approved these changes Aug 31, 2022
View reviewed changes
Copy link
Contributor

@salman90 salman90 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 @derisen
v-michaelmi
v-michaelmi reviewed Aug 31, 2022
View reviewed changes
Copy link
Contributor

@v-michaelmi v-michaelmi left a comment

Choose a reason for hiding this comment

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

Aside from a few nits LGTM
5-AccessControl/1-call-api-roles/SPA/src/app/todo-view/todo-view.component.ts
}

removeTodo(id: string): void {
this.service.deleteTodo(+id).subscribe(() => {
Copy link
Contributor

@v-michaelmi v-michaelmi Aug 31, 2022

Choose a reason for hiding this comment

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

Does the id parameter of deleteTodo have to be a number here?
5-AccessControl/1-call-api-roles/SPA/src/app/todo-edit/todo-edit.component.ts
ngOnInit(): void {
this.route.paramMap.subscribe((params) => {
let id = +params.get('id')!;
this.service.getTodo(+id).subscribe((response: Todo) => {
Copy link
Contributor

@v-michaelmi v-michaelmi Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Same here with id being a number.

Type mismatching like this seems like it might be a possible logic land mine in the future.
Copy link
Contributor Author

@derisen derisen Sep 1, 2022

Choose a reason for hiding this comment

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

id prop is of type int/number in both frontend and backend, so I think we are safe here. The query param is in string so I had to cast it.
kalyankrishna1
kalyankrishna1 approved these changes Sep 1, 2022
View reviewed changes
Copy link
Contributor

@kalyankrishna1 kalyankrishna1 left a comment

Choose a reason for hiding this comment

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

:shipit:
@derisen derisen merged commit 95d7c6a into main Sep 7, 2022
@derisen derisen deleted the basher-5-1 branch September 22, 2022 02:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

5 participants

@derisen @kalyankrishna1 @salman90 @v-michaelmi