Skip to content

Conversation

@vinod827
Copy link
Contributor

Hi,

I have coded a new method called loadFilteredPolicy() to load filtered policies from a given entity repo based on filtered object passed to it.
We are using TypeORM Adapter for Casbin in our project and this is an urgent requirement for us to have this missing functionality so I have added in it and raised PR.

Kindly assist to review my PR and approve it.

Thank you,
Vinod Kumar

@hsluoyz
Copy link
Member

hsluoyz commented May 27, 2020

@nodece @ghaiklor @GopherJ please review.

@nodece
Copy link
Contributor

nodece commented May 27, 2020

Please fix the CI error.

@vinod827
Copy link
Contributor Author

Please fix the CI error.

sorry, is this comment (for CI error) for me to check?....If yes then I tried to analyse as much possible from my side however I did not find anything relevant with respect to my code...I believe this is generic issue of CI/CD pipeline....

@nodece
Copy link
Contributor

nodece commented May 27, 2020

@vinod827 You implement FilteredAdapter.

@nodece
Copy link
Contributor

nodece commented May 27, 2020

How to implement filtered adapter?

We need to abide by the following rules.

loadPolicy

When the method is done, set filtered is false.

loadFilteredPolicy

When the method is done, set filtered is true.

Note: Casbin can not update policy when enable filtered model.

@vinod827
Copy link
Contributor Author

How to implement filtered adapter?

We need to abide by the following rules.

loadPolicy

When the method is done, set filtered is false.

loadFilteredPolicy

When the method is done, set filtered is true.

Note: Casbin can not update policy when enable filtered model.

Is there any example code to set this filtered flag to true?

@vinod827
Copy link
Contributor Author

Hi All,

Please check the build. Only 1 is failing due to some issue.
https://travis-ci.org/github/node-casbin/typeorm-adapter/builds/691784303

Kindly assist to review the PR as well.
Thanks

@nodece
Copy link
Contributor

nodece commented May 27, 2020

Hi @vinod827 , example FilteredAdapter:

class TestAdapter implements FilteredAdapter {
    private filtered = false;

    isFiltered(): boolean {
        return this.filtered;
    }

    async loadPolicy(model: Model): Promise<void> {
        // do something - load policy from db
        
        this.filtered = false;
        return undefined;
    }

    async addPolicy(sec: string, ptype: string, rule: string[]): Promise<void> {
        if (this.filtered) {
            throw new Error("un-support removePolicy in filter model")
        }

        // do something
    }

    async loadFilteredPolicy(model: Model, filter: any): Promise<void> {
        // do something - load policy from db

        this.filtered = true;
    }

    async removeFilteredPolicy(sec: string, ptype: string, fieldIndex: number, ...fieldValues: string[]): Promise<void> {
        if (this.filtered) {
            throw new Error("un-support removePolicy in filter model")
        }

        // do something
    }

    async removePolicy(sec: string, ptype: string, rule: string[]): Promise<void> {
        if (this.filtered) {
            throw new Error("un-support removePolicy in filter model")
        }

        // do something
    }

    async savePolicy(model: Model): Promise<boolean> {
        if (this.filtered) {
            throw new Error("un-support removePolicy in filter model")
        }

        // do something
        return true;
    }
}

```

@nodece
Copy link
Contributor

nodece commented May 27, 2020

@vinod827 You can remove node6 in CI. mysql2 doesn't support node6.

@hsluoyz
Copy link
Member

hsluoyz commented May 28, 2020

Please do not append commits to fix your errors. can you rewrite previous commits?

image

@vinod827
Copy link
Contributor Author

Please do not append commits to fix your errors. can you rewrite previous commits?

image

Sure, I will merge into a larger commit.

['data2_admin', 'data2', 'write']]);

// Load Filtered Policy
/*await a.loadFilteredPolicy(e.getModel(), {
Copy link
Contributor

Choose a reason for hiding this comment

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

@vinod827 Remove the comments. Run it on test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

README.md Outdated
});

// Check the permission.
e.enforce('alice', 'data1', 'read');
Copy link
Contributor

Choose a reason for hiding this comment

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

miss await

Copy link
Contributor Author

@vinod827 vinod827 May 28, 2020

Choose a reason for hiding this comment

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

okay, will add, thanks

src/adapter.ts Outdated
* TypeORMAdapter represents the TypeORM adapter for policy storage.
*/
export default class TypeORMAdapter implements Adapter {
export default class TypeORMAdapter implements Adapter, FilteredAdapter {
Copy link
Contributor

Choose a reason for hiding this comment

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

good: export default class TypeORMAdapter implements FilteredAdapter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

@nodece
Copy link
Contributor

nodece commented May 28, 2020

@vinod827 Why remove the loadFilteredPolicy? You should add some tests for loadFilteredPolicy.

@vinod827
Copy link
Contributor Author

@vinod827 Why remove the loadFilteredPolicy? You should add some tests for loadFilteredPolicy.

okay, will add test, thanks

@vinod827
Copy link
Contributor Author

Hi Team,

I have incorporated all the review comments as suggested and pushed the changes again. This time only build failed due to some other CI error:-
https://travis-ci.org/github/node-casbin/typeorm-adapter/jobs/692144298

I would really appreciate if you can please have a look and approve my PR after review as this functionality is really important and urgent for us.

Thank you,
Regards,
Vinod Kumar

@nodece
Copy link
Contributor

nodece commented May 28, 2020

LGTM, thank @vinod827 for your contribution!

Could you rewrite your commits? It's looks chaos :(

After that, I will merge it into the master.

@hsluoyz
Copy link
Member

hsluoyz commented May 28, 2020

@nodece why the CI is still failing?

@nodece
Copy link
Contributor

nodece commented May 28, 2020

@hsluoyz
Copy link
Member

hsluoyz commented May 28, 2020

@vinod827 @nodece then we should remove node 6 from Travis CI config file.

Copy link
Member

@ghaiklor ghaiklor left a comment

Choose a reason for hiding this comment

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

LGTM

Regard to CI issue, drop the Node.js v6 support from Travis CI. We need to publish it as a major release then.

@hsluoyz hsluoyz merged commit aab0c33 into node-casbin:master May 28, 2020
@hsluoyz
Copy link
Member

hsluoyz commented May 28, 2020

To make it quick, I squashed and merged this PR.

@ghaiklor sounds good.

@nodece can you help remove Node 6 and bump to a new release?

@nodece
Copy link
Contributor

nodece commented May 28, 2020

@vinod827 @hsluoyz v1.2.0 has been released.

@ghaiklor mysql2 doesn't support Node.js v6 but it as `devDependency. Maybe publish it as minor release is better.

@ghaiklor
Copy link
Member

Hm, I thought, mysql is the dependency for the library to operate. Makes sense then to publish as a minor release, if it is just a dev dependency for testing.

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.

4 participants