-
Notifications
You must be signed in to change notification settings - Fork 20
feat(permissions): add permission service for reading scopes and permissions from RPT token #131
feat(permissions): add permission service for reading scopes and permissions from RPT token #131
Conversation
…issions from RPT token
@@ -14,7 +14,7 @@ | |||
"emitDecoratorMetadata": true, | |||
"experimentalDecorators": true, | |||
"inlineSources": true, | |||
"lib": [ "dom", "es6" ], | |||
"lib": [ "dom", "es6", "es2017" ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think this will need to be added to the UI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add it there as well but not necessary since build code from ngx-login-client
will be used in fabric8-ui
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI the monorepo will also include es2017
as part of the common tsconfig
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i did check that tsconfig
in talamer has es2017
along with es2015
and es2016
. :)
src/app/auth/permission.service.ts
Outdated
|
||
@Injectable() | ||
export class PermissionService { | ||
jwtHelper: JwtHelperService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be private?
src/app/auth/permission.service.ts
Outdated
export class PermissionService { | ||
jwtHelper: JwtHelperService; | ||
constructor() { | ||
this.jwtHelper = new JwtHelperService(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the assignment to the declaration.
src/app/auth/permission.service.ts
Outdated
* Checks if the decoded token is valid RPT by checking the permissions claim. | ||
* @param token Decoded JWT token. | ||
*/ | ||
isValidRPT(token: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make private?
src/app/auth/permission.service.ts
Outdated
/** | ||
* Decodes the JWT token using JwtHelperService from `angular-jwt`. | ||
*/ | ||
getDecodedToken() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make private? I see it's used in a test but does it make sense for this to be API?
src/app/auth/permission.service.ts
Outdated
* @param resourceId ID of a specific resource such as a Space | ||
* @param scope the scope you want to check for. Ex - `can edit` | ||
*/ | ||
checkScope(resourceId: string, scope: string): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better name for this API might be isResourceInScope
or isInScope
to align with is
functions returning a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method will be used to check if a user has a specific scope for a resource or not. That way the names like isResourceInScope
or isInScope
doesn't make sense here. Something like hasScopeForResource
or hasScope
might be more meaningful i think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. I'm ok with hasScope
.
src/app/auth/permission.service.ts
Outdated
* Returns the permission for a specific resource. | ||
* @param resourceId ID of a specific resource such as a Space | ||
*/ | ||
getPermission(resourceId: string): Permission { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really need to start using strict nulls so that we can have better type checking. Right now this can return undefined
but that's not clear from the return type.
This is something we will improve with the monorepo linting / tsconfig.
🎉 This PR is included in version 2.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes fabric8-services/fabric8-auth#691
This PR adds
PermissionService
which checks if the token is valid RPT token and adds various methods like -getAllScopes
: Returns all the scopes a user has for a specific resource.checkScope
: Checks if a user has a specific scope for a resource.getPermission
: Returns the permission for a specific resource.Also,
auth0/angular-jwt
instead ofjwt-decode
which is b eing used infabric8-ui
becausejwt-decode
doesn't have any type declarations since its not targetted for typescript or angular specifically which caused build to fail. We should probably make this move forfabric8-ui
as well.es2017
in tsconfig.json to use.includes()
.