feat(permission): add methods for role management and audit RPT logic #132

rohitkrai03 · 2018-11-22T19:12:07Z

As part of the work for 890 and 889 we need

A way to assign user with a specific role.
Get all the users that are assigned a particular role in a space.
Call the Audit RPT API if the current token is not valid RPT or if it does not contain permission information for a particular resource. The Audit endpoint will then return an RPT token with required permission information of a user for a particular resource.

This PR adds the functionality into PermissionService -

assignRole - Assign a specific role to a one or more users in a resource such as space.
getUsersByRole - Get all the users who have a speicific role.
auditRPT - Call the Audit RPT API to get a new RPT token if the token if not an RPT or if the token doesn't contain permission info for a particular resource.

Also,

The logic for getDecodedToken is changed to include a check for valid RPT. If the token is not valid (In case of accessing the resource for the first time when instead of RPT the localStorage will be having auth_token).
Logic for getPermission will also include a call to auditRPT if current token does not include permission information for the required resource. This case may arise when the scope of user has been changed and the RPT is not updated with correct permissions.
Added more tests to include all the new scenarios.

… workflow

christianvogt · 2018-11-29T16:41:43Z

src/app/auth/permission.service.ts

+  getUsersByRole(resourceId: string, roleName: string): Observable<UserRoleData[]> {
+    const url = `${this.authApi}resources/${resourceId}/roles/${roleName}`;
+    return this.http
+      .get(url, { headers: this.headers })


You should parameterize the get function instead of adding type info to the map call.
.get<{ data: UserRoleData[] }>(url, { headers: this.headers })
Then your map becomes:
map(res => res.data)

christianvogt · 2018-11-29T16:42:02Z

src/app/auth/permission.service.ts

+    const url = `${this.authApi}token/audit`;
+    const params = new HttpParams().set('resource_id', resourceId);
+    return this.http
+      .post(url, '', { headers: this.headers, params })


Add type parameters to the the post call instead of in the map.

christianvogt · 2018-11-29T16:43:00Z

src/app/auth/permission.service.ts

-    return permissions ? permissions.scopes : [];
+  getAllScopes(resourceId: string): Observable<string[]> {
+    return this.getPermission(resourceId).pipe(
+      map((permission: Permission) => permission.scopes)


permission may be undefined here.

christianvogt · 2018-11-29T16:43:14Z

src/app/auth/permission.service.ts

-    return permissions ? permissions.scopes.includes(scope) : false;
+  hasScope(resourceId: string, scope: string): Observable<boolean> {
+    return this.getPermission(resourceId).pipe(
+      map((permission: Permission) => permission.scopes.includes(scope))


permission may be undefined here.

christianvogt · 2018-11-29T17:04:19Z

src/app/auth/permission.service.ts

-  private getDecodedToken(): any {
-    const token = localStorage.getItem('auth_token');
-    return token ? this.jwtHelper.decodeToken(token) : '';
+  assignRole(resourceId: string, roleName: string, userIDs: Array<string>): Observable<any> {


Why does this return Observable<any> and not an observable of a specific value or undefined | null ?

christianvogt · 2018-11-29T17:13:16Z

src/app/auth/permission.service.spec.ts

      providers: [
-        PermissionService
+        PermissionService,
+        {


You don't need interceptors for these tests.

christianvogt · 2018-11-29T17:13:44Z

src/app/auth/permission.service.spec.ts

+          useClass: AuthInterceptor,
+          multi: true
+        },
+        { provide: WIT_API_PROXY, useValue: 'http://wit.example.com/'},


You don't need the WIT_API_PROXY or SSO_API_URL for these tests.

christianvogt · 2018-11-29T17:14:00Z

src/app/auth/permission.service.spec.ts

@@ -1,36 +1,183 @@
-import { TestBed } from '@angular/core/testing';
-import { PermissionService, Permission } from './permission.service';
+import { TestBed, async } from '@angular/core/testing';


async import is unused.

…imports

christianvogt · 2018-11-29T22:32:24Z

src/app/auth/permission.service.ts

@@ -47,7 +47,7 @@ export class PermissionService {
   */
  hasScope(resourceId: string, scope: string): Observable<boolean> {
    return this.getPermission(resourceId).pipe(
-      map((permission: Permission) => permission.scopes.includes(scope))
+      map((permission: Permission | undefined) => permission.scopes.includes(scope))


This is still a problem because you use permission without a null check. The old code use to return false if permission was null.

christianvogt · 2018-11-30T15:17:56Z

@rohitkrai03 whenever a test performs assertions in a callback (ie. a subscribe), you need to ensure that the callback is executed. Otherwise the test can still pass even though your assertions weren't executed. Most tests have been implemented using done to notify when the test completes.

fabric8cd · 2018-11-30T19:50:10Z

🎉 This PR is included in version 2.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat(permission): add methods for role management and audit RPT logic

71f273d

rohitkrai03 requested a review from christianvogt November 22, 2018 19:12

rohitkrai03 force-pushed the manage-roles branch from 519015a to 1640fa9 Compare November 23, 2018 19:51

fix(permission): fix assign role url and http method

dfa6403

rohitkrai03 force-pushed the manage-roles branch from 1640fa9 to dfa6403 Compare November 23, 2018 20:00

rohitkrai03 changed the title ~~feat(permission): add methods for role management and audit RPT logic~~ [WIP] feat(permission): add methods for role management and audit RPT logic Nov 25, 2018

rohitkrai03 removed the request for review from christianvogt November 25, 2018 14:15

rohitkrai03 changed the title ~~[WIP] feat(permission): add methods for role management and audit RPT logic~~ feat(permission): add methods for role management and audit RPT logic Nov 28, 2018

rohitkrai03 requested a review from christianvogt November 28, 2018 20:38

fix(permission): use rxjs observables for handling asynchronous audit…

266fd15

… workflow

rohitkrai03 force-pushed the manage-roles branch from 5280ed6 to 266fd15 Compare November 29, 2018 07:57

christianvogt suggested changes Nov 29, 2018

View reviewed changes

fix(permission): explicit return types of requests and remove unused …

78173a4

…imports

christianvogt reviewed Nov 29, 2018

View reviewed changes

fix(permission): undefined check in hasScope and getAllScopes

7b53350

fix(permission): add cllaback done check within subscribe

8bf3eee

christianvogt approved these changes Nov 30, 2018

View reviewed changes

christianvogt merged commit abc6eb3 into fabric8-ui:master Nov 30, 2018

fabric8cd added the released label Nov 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(permission): add methods for role management and audit RPT logic #132

feat(permission): add methods for role management and audit RPT logic #132

rohitkrai03 commented Nov 22, 2018

christianvogt Nov 29, 2018

christianvogt Nov 29, 2018

christianvogt Nov 29, 2018

christianvogt Nov 29, 2018

christianvogt Nov 29, 2018

christianvogt Nov 29, 2018

christianvogt Nov 29, 2018

christianvogt Nov 29, 2018

christianvogt Nov 29, 2018

christianvogt commented Nov 30, 2018

fabric8cd commented Nov 30, 2018

feat(permission): add methods for role management and audit RPT logic #132

feat(permission): add methods for role management and audit RPT logic #132

Conversation

rohitkrai03 commented Nov 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Nov 30, 2018

fabric8cd commented Nov 30, 2018