Configurable Keto variables #870

AnujaVane · 2020-07-09T16:14:47Z

What this PR does / why we need it:

Configurable permission role templates.
Configurable subject from the JWT token.

Does this PR introduce a user-facing change?: NONE

feast-ci-bot · 2020-07-09T16:14:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AnujaVane
To complete the pull request process, please assign davidheryanto
You can assign the PR to them by writing /assign @davidheryanto in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

feast-ci-bot · 2020-07-09T16:15:02Z

Hi @AnujaVane. Thanks for your PR.

I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dr3s · 2020-07-09T16:33:33Z

run mvn spotless:apply

dr3s · 2020-07-09T19:10:58Z

auth/src/main/java/feast/auth/authorization/Keto/KetoAuthorizationProvider.java

    if (options == null) {
      throw new IllegalArgumentException("Cannot pass empty or null options to KetoAuth");
    }
    ApiClient defaultClient = Configuration.getDefaultApiClient();
-    defaultClient.setBasePath(options.get("basePath"));
+    defaultClient.setBasePath((String) options.get("basePath"));


a better name for the option is authorizationUrl

dr3s · 2020-07-09T19:11:51Z

auth/src/main/java/feast/auth/authorization/Keto/KetoAuthorizationProvider.java

@@ -57,20 +59,28 @@ public KetoAuthorizationProvider(Map<String, String> options) {
   * @return AuthorizationResult result of authorization query
   */
  public AuthorizationResult checkAccess(String project, Authentication authentication) {
-    String email = getEmailFromAuth(authentication);
+    String subject = (String) this.options.get("subject");
+    if ((subject == null) || (subject.isEmpty())) subject = "email";


email should be a constant like DEFAULT_SUBJECT_CLAIM

dr3s · 2020-07-09T19:12:16Z

auth/src/main/java/feast/auth/authorization/Keto/KetoAuthorizationProvider.java

@@ -57,20 +59,28 @@ public KetoAuthorizationProvider(Map<String, String> options) {
   * @return AuthorizationResult result of authorization query
   */
  public AuthorizationResult checkAccess(String project, Authentication authentication) {
-    String email = getEmailFromAuth(authentication);
+    String subject = (String) this.options.get("subject");


A better name for this option is subjectClaim

rename the option and the variable actually

dr3s · 2020-07-09T19:14:54Z

auth/src/main/java/feast/auth/authorization/Keto/KetoAuthorizationProvider.java

   * @return String user email
   */
-  private String getEmailFromAuth(Authentication authentication) {
+  private String getSubjectFromAuth(Authentication authentication, String subject) {


rename subject to subjectClaim

dr3s · 2020-07-09T19:15:24Z

auth/src/main/java/feast/auth/authorization/Keto/KetoAuthorizationProvider.java

    Jwt principle = ((Jwt) authentication.getPrincipal());
    Map<String, Object> claims = principle.getClaims();
-    String email = (String) claims.get("email");
+    String subjectValue = (String) claims.get(subject);


subjectValue can then be named subject

dr3s · 2020-07-09T19:16:17Z

auth/src/main/java/feast/auth/config/SecurityProperties.java

@@ -54,6 +54,6 @@
    private String provider;

    // K/V options to initialize the provider with


this is no longer a k/V pair since we allow a list for roles. may be worth a conversation

dr3s · 2020-07-09T19:17:12Z

auth/src/main/java/feast/auth/authorization/Keto/KetoAuthorizationProvider.java

-            || (String.format("roles:feast:%s-member", project)).equals(role.getId())) {
-          return AuthorizationResult.success();
+      for (String roleTemplate : roleTemplates) {
+        if (roleTemplate.contains("{PROJECT}")) {


PROJECT could be a constant and actually could be in a list of constants that could be replaced in the role templates.

dr3s · 2020-07-09T19:17:47Z

/lgtm

dr3s · 2020-07-09T19:17:57Z

/ok-to-test

woop · 2020-07-10T01:29:37Z

core/src/main/resources/application.yml

+        subject: email
+        roles:
+          - role:feast:admin
+          - role:feast:{PROJECT}-member


I am a bit wary about using complex objects as options. I would strongly prefer to stick to K/V here, so perhaps we can turn roles into a CSV.

woop · 2020-07-10T01:36:01Z

I think a fundamental problem that I have with the current checkAccess implementation for Keto is that it doesn't use Keto as intended. Yes, we can retrieve the roles and parse it client-side to determine access, but that is what the /allowed endpoint on Keto is for. In a sense, I think we are duplicating work by adding role templating into Feast. Keto already supports this through its pattern matching.

woop · 2020-07-10T08:21:11Z

/ok-to-test

feast-ci-bot · 2020-07-10T08:41:05Z

@AnujaVane: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
test-end-to-end-batch	`1da5201`	link	`/test test-end-to-end-batch`
test-end-to-end-auth	`1da5201`	link	`/test test-end-to-end-auth`

Full PR test history

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

dr3s · 2020-07-10T14:58:26Z

replaced with #864

Anuja Vane and others added 2 commits July 8, 2020 09:20

configurable roles and claim

4f63b65

minor refactoring

a14d4b6

AnujaVane requested review from davidheryanto, khorshuheng, pyalex, woop and zhilingc as code owners July 9, 2020 16:14

feast-ci-bot added the do-not-merge/work-in-progress label Jul 9, 2020

feast-ci-bot added the needs-kind label Jul 9, 2020

feast-ci-bot added size/M needs-ok-to-test labels Jul 9, 2020

AnujaVane changed the title ~~WIP : Feature/keto config~~ WIP: Configurable Keto variables Jul 9, 2020

fixed build and lint

1da5201

AnujaVane changed the title ~~WIP: Configurable Keto variables~~ Configurable Keto variables Jul 9, 2020

feast-ci-bot removed the do-not-merge/work-in-progress label Jul 9, 2020

dr3s reviewed Jul 9, 2020

View reviewed changes

feast-ci-bot assigned dr3s Jul 9, 2020

feast-ci-bot added the lgtm label Jul 9, 2020

feast-ci-bot added the ok-to-test label Jul 9, 2020

feast-ci-bot removed the needs-ok-to-test label Jul 9, 2020

woop reviewed Jul 10, 2020

View reviewed changes

dr3s closed this Jul 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable Keto variables #870

Configurable Keto variables #870

AnujaVane commented Jul 9, 2020

feast-ci-bot commented Jul 9, 2020

feast-ci-bot commented Jul 9, 2020

dr3s commented Jul 9, 2020

dr3s Jul 9, 2020

dr3s Jul 9, 2020

dr3s Jul 9, 2020

dr3s Jul 9, 2020

dr3s Jul 9, 2020

dr3s Jul 9, 2020

dr3s Jul 9, 2020

dr3s Jul 9, 2020

dr3s commented Jul 9, 2020

dr3s commented Jul 9, 2020

woop Jul 10, 2020

woop commented Jul 10, 2020

woop commented Jul 10, 2020

feast-ci-bot commented Jul 10, 2020

dr3s commented Jul 10, 2020

		@@ -54,6 +54,6 @@
		private String provider;

		// K/V options to initialize the provider with

Configurable Keto variables #870

Configurable Keto variables #870

Conversation

AnujaVane commented Jul 9, 2020

feast-ci-bot commented Jul 9, 2020

feast-ci-bot commented Jul 9, 2020

dr3s commented Jul 9, 2020

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

dr3s commented Jul 9, 2020

dr3s commented Jul 9, 2020

Choose a reason for hiding this comment

woop commented Jul 10, 2020

woop commented Jul 10, 2020

feast-ci-bot commented Jul 10, 2020

dr3s commented Jul 10, 2020