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

[WIP] feat: add authorization component #1205

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@raymondfeng
Copy link
Member

commented Mar 28, 2018

Connect to #538

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in packages/example-* were updated

@raymondfeng raymondfeng requested a review from bajtos as a code owner Mar 28, 2018

authorize({allow: ['$authenticated']});
export const denyAll = () => authorize({deny: ['$everyone']});
export const denyUnauthenticated = () =>
authorize({deny: ['$anonymous', 'unauthenticated']});

This comment has been minimized.

Copy link
@virkt25

virkt25 Mar 28, 2018

Contributor

$unauthenticated?

This comment has been minimized.

Copy link
@JesusTheHun

JesusTheHun Oct 11, 2018

What is the difference between $anonymous and $unauthenticated ?

This comment has been minimized.

Copy link
@JesusTheHun

JesusTheHun Oct 11, 2018

One must be careful while implementing the all roles. Very often what the developer wants is all but . A developer may be tempted to write something like :

@authorize({allow: ['MODERATOR']})
@authorize.denyAll()
// maybe a helper like this may be wanted
@authorize.denyAllBut(['MODERATOR'])
// or of course
@authorize.allowAllBut(['$anonymous'])
myMethod();

This is a case to handle. The order of annotation is very important and counter intuitive so the documentation must warn the user about that. Hence the helper denyAllBut();

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Oct 17, 2018

Author Member

There should be a rule that makes the order of decorators irrelevant.

@raymondfeng raymondfeng force-pushed the authorization branch 2 times, most recently from b70c54c to 23a85a2 Apr 9, 2018

## Basic use

Start by decorating your controller methods with `@authorize` to require the
request to be authenticated.

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 17, 2018

Member

authenticated or authorized?

this.providers = {
[AuthorizationBindings.AUTHORIZE_ACTION.key]: AuthorizationProvider,
[AuthorizationBindings.AUTHORIZE_METADATA
.key]: AuthorizationMetadataProvider,

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 17, 2018

Member

I find this formatting rather weird, was it created by prettier?

securityContext: SecurityContext,
metadata: AuthorizationMetadata,
) => {
return AuthorizationDecision.ALLOW;

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 17, 2018

Member

Is this correct? Shouldn't the reference implementation actually handle $everyone, $unauthenticated and $anonymous roles/scopes as defined in packages/authorization/src/decorators/authorize.ts?

authorize({allow: ['$authenticated']});
export const denyAll = () => authorize({deny: ['$everyone']});
export const denyUnauthenticated = () =>
authorize({deny: ['$anonymous', '$unauthenticated']});

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 17, 2018

Member

What is the difference between anonymous and unauthenticated?

@bajtos bajtos removed the review label Jun 25, 2018

@raymondfeng raymondfeng force-pushed the authorization branch 2 times, most recently from 1f6e799 to 2b66269 Aug 20, 2018

@dhmlau dhmlau referenced this pull request Oct 11, 2018

Closed

[Roadmap] 🚀1Q2019 🚀 #1839

14 of 20 tasks complete
describe('@authorize decorator', () => {
it('can add authorize metadata to target method', () => {
class TestClass {
@authorize({allow: ['ADMIN'], scopes: ['secret.read']})

This comment has been minimized.

Copy link
@JesusTheHun

JesusTheHun Oct 11, 2018

I'm confused by the definition of scope and the role based definition in the same decorator.
@scope('secret.read') this is my scope definition
@role({ allow: ['ADMIN']}) this is my role based access definition

These are two completely different things.

Scenario 1 : I use role based authorization, I decorate my route to define who can access to this route. This is @role.
Scenario 2 : I use scope based authorization, my user is allowed to access a set of scopes, coming from database or token ; if the route he tries to access is within his scopes, access is granted. This is @scope
Scenario 3 : I use both, every role has a set of scope associated, hard coded or defined through user land configuration. This is @scope ; in my authentication configuration I bind each role to a set of scopes.


it('can add denyAll to target method', () => {
class TestClass {
@authorize.denyAll()

This comment has been minimized.

Copy link
@JesusTheHun

JesusTheHun Oct 11, 2018

From a security perspective, denyAll() should be the default behavior. If the user wants a whitelist approach, it should be able to set it in the component configuration

@JesusTheHun

This comment has been minimized.

Copy link

commented Oct 11, 2018

For future releases, it will be nice to bring a context based authorization. Sometimes referred as a voter approach.

@vote({ voter: 'binding.myService', identifier: 'articleId' })
// or with helper
@voteFromRepository({ voter: ArticleRepository, identifier: 'articleId' })
@scope('article.write');
myWriteMethod(articleId: string);
class ArticleRepository implements Voter {}
interface Voter {
    public vote(user: MyUserModel, articleId: string, scope: string): boolean
}

The authorization decorator retrieve the articleId at runtime, ask the ArticleRepository to tell is this user is allowed to perform action on this very article, within the provided scope. The MyUserModel instance is injected by asking the authentication component.

@raymondfeng raymondfeng force-pushed the authorization branch 3 times, most recently from 3046254 to cf5b9c9 Oct 17, 2018

@raymondfeng raymondfeng force-pushed the authorization branch from cf5b9c9 to 096f20e Oct 31, 2018

@JesusTheHun

This comment has been minimized.

Copy link

commented Nov 7, 2018

Is there any progress on this subject ?

We would like to launch several API using LB4 in the next months but it lacks authorization and authentication components.

edit : Is it realistic to expect these two components to be added to the December milestone ?

@bajtos

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

Is there any progress on this subject ?

Right now, we are trying to figure out a plan how to approach the large task of figuring out authorization implementation. I summarized my current thinking in #1035 (comment), but it's not definitive yet.

We would like to launch several API using LB4 in the next months but it lacks authorization and authentication components.
edit : Is it realistic to expect these two components to be added to the December milestone ?

Considering our current velocity and also the fact that many of us will be taking time off (vacations) in December, I find it very unlikely to have production-level implementation of authentication and authorization components done by the end of 2018.

@JesusTheHun

This comment has been minimized.

Copy link

commented Nov 8, 2018

Thank you for your response.
Do you see anything I can do to help you on this path ?

@bajtos

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

@JesusTheHun thank you for the offer! I am afraid this work is currently blocked by missing hasOne relation that @b-admike is working on right now - see #1879. Once hasOne is landed, you can help us with #1996, #1997 and #1998. But be warned, this is an exploratory work that is likely to discover missing pieces. These missing pieces may be non-trivial to add and requiring design discussions first.

@raymondfeng raymondfeng force-pushed the authorization branch from 096f20e to f9f0d70 Apr 22, 2019

@raymondfeng raymondfeng force-pushed the authorization branch from f9f0d70 to a24807f May 10, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

I rewrote the code based on interceptors.

@raymondfeng raymondfeng force-pushed the authorization branch from c32a1d5 to 54f8510 Jul 1, 2019

@raymondfeng raymondfeng referenced this pull request Jul 8, 2019

Merged

docs: add Q3 roadmap #3152

0 of 7 tasks complete

@dhmlau dhmlau added the 2019Q3 label Jul 8, 2019

@dhmlau dhmlau referenced this pull request Jul 9, 2019

Open

Monthly Milestone - Jul 2019 🌴⚽️🍒 #3241

11 of 30 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.