Default AuthZ Provider Skeleton Code #45

aayustark007-fk · 2023-09-03T16:48:27Z

Implemented config driven roles and role binding
Linking to the AuthorizationProvider interface and implemented isAuthorised method
Added some basic roles and role bindings

codecov-commenter · 2023-09-03T16:52:07Z

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (c066395) 52.58% compared to head (578ab12) 53.55%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master      #45      +/-   ##
============================================
+ Coverage     52.58%   53.55%   +0.97%     
- Complexity      226      245      +19     
============================================
  Files            72       72              
  Lines          1394     1449      +55     
  Branches         83       85       +2     
============================================
+ Hits            733      776      +43     
- Misses          631      640       +9     
- Partials         30       33       +3

Files	Coverage Δ
...m/flipkart/varadhi/auth/AuthorizationProvider.java	`0.00% <ø> (ø)`
...n/java/com/flipkart/varadhi/auth/ResourceType.java	`0.00% <0.00%> (ø)`
...art/varadhi/auth/DefaultAuthorizationProvider.java	`93.47% <93.47%> (ø)`
...in/java/com/flipkart/varadhi/web/AuthHandlers.java	`0.00% <0.00%> (ø)`
...java/com/flipkart/varadhi/auth/ResourceAction.java	`0.00% <0.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2023-09-03T16:53:23Z

Test Results

90 tests 90 ✔️ 42s ⏱️
25 suites   0 💤
25 files   0 ❌

Results for commit 398a342.

♻️ This comment has been updated with latest results.

…into default-authz

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

kmrdhruv · 2023-09-11T16:17:25Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

+        // TODO(aayush): need better way to do this?
+        Map<ResourceType, String> resultMap = new HashMap<>();
+        String[] segments = resource.split("/");
+        return resolve(ResourceType.ORG, action, List.of(segments), resultMap);


Since format is fixed and resolve also depends on the format, would it be better to do simple parsing instead of recursive call ?

agree, will make the changes based on the outcome of #45 (comment)

kmrdhruv · 2023-09-11T16:18:36Z

server/src/main/resources/configuration.yml


 authorization:
  superUsers: [ "thanos" ]
-  providerClassName:
-  providerOptions:
+  providerClassName: "com.flipkart.varadhi.auth.DefaultAuthorizationProvider"


fine for POC, but for prod it might be better to extract these to authz specific file and have a pointer in the main config file.

sure will do that

kmrdhruv · 2023-09-11T16:27:21Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

+    @Override
+    public Future<Boolean> isAuthorized(UserContext userContext, ResourceAction action, String resource) {
+        // parse the resource path based on the action and return the final resourceIDs for each resourceType
+        Map<ResourceType, String> resourceTypeToResourceId = getResourceIds(action, resource);


would it be more beneficial to evaluate it from root to leaf or opposite, thoughts ?

Currently we iterate over map entries, so the evaluation order here is undefined.

One problem them I'm facing is that in case of project, how do I figure out the parent entity ID?

Some approaches I have in mind:

Pass only the project_id as resource (as being currently done in RequiredAuthorization) and the Authorization Provider has the required mapping to the parent entity. So, when we fetch the role_bindings for a given resource_id, we get the role_bindings at that resource_id level along with the role_bindings at the parent level till the root node.

Or, since this is a Default barebones impl, we just do away with the requirement of inheriting permissions and check only at the resource_ids being passed.

Now coming to the original question, what would be better? Root to Leaf or opposite?
I'd say, considering the points above, opposite evaluation will be much better as we are mostly being passed the last set of identifiers through which we will mostly move up the hierarchy. Evaluating Root to Leaf will involve the extra work of figuring out the entire hierarchy at once and then evaluating.

What are your thoughts?

Updated the code, we are evaluating in lazy manner from leaf to root node

kmrdhruv · 2023-09-11T16:34:18Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

+    }
+
+    private boolean doesActionBelongToRole(String roleId, ResourceAction action) {
+        return configuration.getRoles().getOrDefault(roleId, new ArrayList<>()).contains(action);


for debugging purpose, it would be good maintain (log) the information which node and which role returned true for authorised call.

…into default-authz

aayustark007-fk · 2023-10-13T13:53:43Z

entities/src/main/java/com/flipkart/varadhi/auth/ResourceType.java

@@ -1,6 +1,7 @@
 package com.flipkart.varadhi.auth;

 public enum ResourceType {
+    ROOT("root"),


required for bootstrapping and org create

How is boot strapping being done ? Also let's finalise how we want to provide org creation ?

Idea is that when there is no Org defined, we still need a resource node to hold the bootstrap role bindings. Thats why a ROOT is defined which will hold role bindings required for org creation and role assignment. We can also drive super-users by adding role bindings to this node.

aayustark007-fk · 2023-10-13T14:13:38Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

+    private volatile boolean initialised = false;
+
+    @Override
+    public Future<Boolean> init(AuthorizationOptions authorizationOptions) {


Was planning to read the file as json object and pass to the providers. But, decided against it because I think it would be better to have control at the provider end in case they wish to specify the configuration as JSON instead of YAML or even interpret the configFile path as an URI and fetch the config over the internet or file server.

with this we can also give configFile as:
configFile: "https://raw.githubusercontent.com/acme-org/acme-project/customconfig.json"

And the provider can then make an http call to fetch and parse the config.

I think we can stay with single file format for entire project instead of giving the flexibility. Thoughts ?
However loading from different location is good to have feature. We can look to enhance yaml loader to support these, may be at later point ?

entities/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationConfiguration.java

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

kmrdhruv · 2023-10-16T11:23:38Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

+        // build the list in reverse order specified: ROOT -> ORG -> TEAM -> PROJECT -> TOPIC|SUBSCRIPTION|QUEUE
+        List<Pair<ResourceType, String>> resourceIdTuples = new ArrayList<>();
+        // handle leaf node case
+        if (isActionOnLeafNode(action)) {


does leaf node need to be special case ?

Not a special case, we want to know if there are any topic or subscription id provided in the resource path. Eg: Say in case of project create, there will be no topic or subscription id present so no leaf node is added in the list of tuples.

kmrdhruv · 2023-10-16T11:24:37Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

+        if (isActionOnLeafNode(action)) {
+            resourceIdTuples.add(Pair.of(action.getResourceType(), getLeaf(segments)));
+        }
+        resourceIdTuples.add(Pair.of(ResourceType.PROJECT, getProject(segments)));


does empty values needs to be populated for non-exising node types in the path ? This can be what is given in the input and also avoid isNotBlank() check in isAuthorized() above ?

I think we can omit adding keys which are blank. Will make the change

kmrdhruv · 2023-10-16T11:26:26Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

+
+    private boolean doesActionBelongToRole(String subject, String roleId, ResourceAction action) {
+        log.debug("Evaluating action [{}] for subject [{}] against role [{}]", action, subject, roleId);
+        boolean matching = configuration.getRoles().getOrDefault(roleId, List.of()).contains(action.name());


Can role be implemented as Set<>, instead of List<> of actions ?

server/src/main/resources/configuration.yml

kmrdhruv · 2023-10-16T11:33:42Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

+    private volatile boolean initialised = false;
+
+    @Override
+    public Future<Boolean> init(AuthorizationOptions authorizationOptions) {


I think we can stay with single file format for entire project instead of giving the flexibility. Thoughts ?
However loading from different location is good to have feature. We can look to enhance yaml loader to support these, may be at later point ?

kmrdhruv · 2023-10-16T15:26:43Z

entities/src/main/java/com/flipkart/varadhi/auth/ResourceType.java

@@ -1,6 +1,7 @@
 package com.flipkart.varadhi.auth;

 public enum ResourceType {
+    ROOT("root"),


How is boot strapping being done ? Also let's finalise how we want to provide org creation ?

kmrdhruv · 2023-10-16T16:04:14Z

server/src/main/java/com/flipkart/varadhi/auth/AuthorizationOptions.java

+     * If specified as true, then the default implementation via {@link com.flipkart.varadhi.auth.DefaultAuthorizationProvider} is used.<br/>
+     * {@link AuthorizationOptions#providerClassName} value is ignored.
+     */
+    private Boolean useDefaultProvider;


is useDefaultProvider redundant given that provierClassName exists ? Also let's see how should we enable superUsers ?

I think we can remove the useDefaultProvider flag since providerClassName will be enough to flag that case

kmrdhruv · 2023-10-16T16:09:55Z

server/src/main/java/com/flipkart/varadhi/web/AuthHandlers.java

            return new AuthorizationHandlerBuilder(configuration.getAuthorization()
                    .getSuperUsers(), authorizationProvider);
        } else {
            return null;
        }
    }

+    private AuthorizationProvider getAuthorizationProvider(ServerConfiguration configuration) {
+        if (configuration.getAuthorization().getUseDefaultProvider()) {


any specific reason to special case for this when compared to others ?

…into default-authz

kmrdhruv

LGTM

aayustark007-fk added 3 commits September 3, 2023 18:40

create basic code for default authz

760acbe

add all permissions

ad2568e

added default authorization provider impl

2a2f5d6

aayustark007-fk changed the title ~~Default AuthZ Provider Skeleton Code~~ <wip> Default AuthZ Provider Skeleton Code Sep 3, 2023

remvoe unused import

4fa1f91

aayustark007-fk added 4 commits September 4, 2023 14:22

fix configs

c309126

remove dummy code

1a7a32c

Merge branch 'master' of https://github.com/flipkart-incubator/varadhi …

bc5424a

…into default-authz

remove usage of tenant

d8d3d70

kmrdhruv requested changes Sep 11, 2023

View reviewed changes

aayustark007-fk added 8 commits September 23, 2023 23:26

address some PR comments

43ee29f

Merge branch 'master' of https://github.com/flipkart-incubator/varadhi …

c904085

…into default-authz

code refactoring to simplify logic

b909499

add commons lang3

a41169a

ensure authz is resolved in ordered manner from leaf nodes to root nodes

146e423

config flag to use default auth provider

d31c53b

drive authz config via ref to config file

1f78461

add tests for default authz provider

149840f

aayustark007-fk commented Oct 13, 2023

View reviewed changes

aayustark007-fk added 2 commits October 13, 2023 19:30

remove unused imports and cleanup code

a7f8797

small doc update

2bc313d

aayustark007-fk changed the title ~~<wip> Default AuthZ Provider Skeleton Code~~ Default AuthZ Provider Skeleton Code Oct 13, 2023

aayustark007-fk requested a review from kmrdhruv October 13, 2023 14:09

aayustark007-fk commented Oct 13, 2023

View reviewed changes

kmrdhruv requested changes Oct 16, 2023

View reviewed changes

aayustark007-fk added 3 commits October 17, 2023 11:50

simplify logic to resolve authz heirarchy

655b0e0

replace snakeyaml with jackson yaml factory object mapper

7c707ba

Merge branch 'master' of https://github.com/flipkart-incubator/varadhi …

578ab12

…into default-authz

aayustark007-fk added 2 commits October 25, 2023 14:55

remove ROOT and make List to Set for default authz config

8457848

resolve PR comments

398a342

kmrdhruv approved these changes Oct 25, 2023

View reviewed changes

aayustark007-fk merged commit f2179e3 into master Oct 26, 2023
2 checks passed

aayustark007-fk mentioned this pull request Dec 13, 2023

Implement API for Default AuthZ provider to provision and modify permissions #54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default AuthZ Provider Skeleton Code #45

Default AuthZ Provider Skeleton Code #45

aayustark007-fk commented Sep 3, 2023 •

edited

codecov-commenter commented Sep 3, 2023 •

edited

github-actions bot commented Sep 3, 2023 •

edited

kmrdhruv Sep 11, 2023

aayustark007-fk Sep 23, 2023

kmrdhruv Sep 11, 2023

aayustark007-fk Sep 23, 2023

kmrdhruv Sep 11, 2023

aayustark007-fk Sep 23, 2023

aayustark007-fk Oct 13, 2023

kmrdhruv Sep 11, 2023

aayustark007-fk Sep 23, 2023

aayustark007-fk Oct 13, 2023

kmrdhruv Oct 16, 2023

aayustark007-fk Oct 16, 2023

aayustark007-fk Oct 13, 2023

aayustark007-fk Oct 13, 2023

kmrdhruv Oct 16, 2023

kmrdhruv Oct 16, 2023

aayustark007-fk Oct 16, 2023

kmrdhruv Oct 16, 2023

aayustark007-fk Oct 17, 2023

kmrdhruv Oct 16, 2023

aayustark007-fk Oct 18, 2023

kmrdhruv Oct 16, 2023

kmrdhruv Oct 16, 2023

kmrdhruv Oct 16, 2023

aayustark007-fk Oct 18, 2023

kmrdhruv Oct 16, 2023

kmrdhruv left a comment

Default AuthZ Provider Skeleton Code #45

Default AuthZ Provider Skeleton Code #45

Conversation

aayustark007-fk commented Sep 3, 2023 • edited

codecov-commenter commented Sep 3, 2023 • edited

Codecov Report

github-actions bot commented Sep 3, 2023 • edited

Test Results

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

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

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

kmrdhruv left a comment

Choose a reason for hiding this comment

aayustark007-fk commented Sep 3, 2023 •

edited

codecov-commenter commented Sep 3, 2023 •

edited

github-actions bot commented Sep 3, 2023 •

edited