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.

Sign up for GitHub

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

Jump to bottom

[Feature/Identity] User operations: create update delete #4741

Merged
merged 11 commits into from
Nov 14, 2022
Merged

[Feature/Identity] User operations: create update delete #4741

merged 11 commits into from
Nov 14, 2022

Conversation

DarshitChanpura
Copy link
Member

@DarshitChanpura DarshitChanpura commented Oct 11, 2022 •
edited
Loading

Signed-off-by: Darshit Chanpura dchanp@amazon.com

Description

Adds C, U, D operations for internal user in an in-memory store.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@DarshitChanpura DarshitChanpura requested review from a team and reta as code owners October 11, 2022 20:09
@DarshitChanpura DarshitChanpura force-pushed the subject-create-update-delete branch from 71a29f3 to 33e8d47 Compare October 11, 2022 20:13
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@DarshitChanpura DarshitChanpura marked this pull request as draft October 11, 2022 20:13
@DarshitChanpura DarshitChanpura changed the title Subject create update delete [Feature/Identity] User operations: create update delete Oct 11, 2022
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@DarshitChanpura DarshitChanpura force-pushed the subject-create-update-delete branch from 33e8d47 to 56db04e Compare October 11, 2022 20:38
@DarshitChanpura DarshitChanpura mentioned this pull request Oct 11, 2022
Closed
6 tasks
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

Codecov Report

Merging #4741 (ffff298) into feature/identity (90cc18a) will decrease coverage by 0.13%.
The diff coverage is 85.60%.

@@                  Coverage Diff                   @@
##             feature/identity    #4741      +/-   ##
======================================================
- Coverage               70.64%   70.50%   -0.14%     
+ Complexity              57685    57631      -54     
======================================================
  Files                    4675     4680       +5     
  Lines                  277347   277478     +131     
  Branches                40556    40562       +6     
======================================================
- Hits                   195935   195642     -293     
- Misses                  64951    65428     +477     
+ Partials                16461    16408      -53
Impacted Files Coverage Δ
...rc/main/java/org/opensearch/authn/AccessToken.java 0.00% <ø> (ø)
...src/main/java/org/opensearch/authn/Principals.java 100.00% <ø> (ø)
...ensearch/identity/noop/NoopAccessTokenManager.java 0.00% <ø> (ø)
...earch/identity/noop/NoopAuthenticationManager.java 66.66% <ø> (ø)
...java/org/opensearch/identity/noop/NoopSubject.java 54.54% <0.00%> (-12.13%) ⬇️
...java/org/opensearch/authn/DefaultObjectMapper.java 62.50% <62.50%> (ø)
...authn/src/main/java/org/opensearch/authn/User.java 85.00% <85.00%> (ø)
...org/opensearch/authn/realm/InternalUsersStore.java 85.00% <85.00%> (ø)
...java/org/opensearch/authn/realm/InternalRealm.java 92.53% <92.53%> (ø)
...ain/java/org/opensearch/authn/StringPrincipal.java 90.00% <100.00%> (ø)
... and 498 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

peternied
peternied reviewed Oct 12, 2022
View reviewed changes
sandbox/libs/authn/src/main/java/org/opensearch/authn/User.java Outdated
import java.util.Map;

public class User {
public class User implements Subject {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think User should be a subject since its (de)serialized, I think we should have a sperate InternalSubject that references an instance of User, what do you think of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I might be a bit confused here. In my understanding, a User is one of the many types of Subject, and InternalSubject and Subject were same. Let me know if my understanding is correct.
Also, w.r.t. serialization/deserialization, isn't the PrincipalIdentifierToken the only serializable entity? Correct me if my understanding is off

Copy link
Member

Choose a reason for hiding this comment

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

User represents the storage medium, Subject has functionality built onto it, and while the principal has strong alignment with the user, I would recommend we create another class to be a Subject that can swap out which User principal is pointed to. Take a look at Shiros tutorial [1], its usage mirrors closely what I think we should implement here. What do you think?

[1] https://shiro.apache.org/10-minute-tutorial.html

Copy link
Member Author

Choose a reason for hiding this comment

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

okay..makes sense... I will update this PR accordingly

@DarshitChanpura DarshitChanpura marked this pull request as ready for review November 8, 2022 18:10
@DarshitChanpura DarshitChanpura marked this pull request as draft November 8, 2022 18:13
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

@DarshitChanpura DarshitChanpura force-pushed the subject-create-update-delete branch from 29a76be to 808eea8 Compare November 9, 2022 16:02
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

@DarshitChanpura
Copy link
Member Author

DarshitChanpura commented Nov 9, 2022 •
edited
Loading

Precommit task :server:dependencyLicenses fails with unused licenses:

* What went wrong:
Execution failed for task ':server:dependencyLicenses'.
> Unused sha files found: 
  lucene-queries-9.4.0.jar.sha1
  lucene-memory-9.4.0.jar.sha1
  lucene-analysis-common-9.4.0.jar.sha1
  lucene-grouping-9.4.0.jar.sha1
  lucene-highlighter-9.4.0.jar.sha1
  lucene-misc-9.4.0.jar.sha1
  lucene-spatial3d-9.4.0.jar.sha1
  lucene-spatial-extras-9.4.0.jar.sha1
  lucene-join-9.4.0.jar.sha1
  lucene-core-9.4.0.jar.sha1
  lucene-sandbox-9.4.0.jar.sha1
  lucene-queryparser-9.4.0.jar.sha1
  lucene-backward-codecs-9.4.0.jar.sha1
  lucene-suggest-9.4.0.jar.sha1

Similar reason for Check task plugins:analysis-icu:dependencyLicenses failure too:

* What went wrong:
Execution failed for task ':plugins:analysis-icu:dependencyLicenses'.
> Unused sha files found: 
  lucene-analysis-icu-9.4.0.jar.sha1

How should we proceed about updating these licenses?
Running ./gradlew updateShas

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

cwperks and others added 4 commits November 9, 2022 14:45
@cwperks @DarshitChanpura
Add line item in CHANGELOG
7b2c56c 
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@DarshitChanpura
Adds Create, Update, Delete operations for subject and adds tests for…
25173db 
… those operations

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Addresses PR comments and changes signature of create and delete methods
7ba0283 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Addresses PR comments
97c8c0d 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Rebases with source branch and updates some method signatures as well…
38623f6 
… as adds a couple of tests

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Deletes incorrectly commit file
6a015c6 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Adds this PR to changelog
e57429f 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Adds missing javadoc
2141624 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Makes user a data storage medium only and moves all crud logic in the…
74b19ed 
… realm itself

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Fixes gradle precommit failures
63332ee 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Fixes thirdPartyAudit task failure by update list of ignoredClasses
13034d1 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura DarshitChanpura force-pushed the subject-create-update-delete branch from e4fee4b to 13034d1 Compare November 9, 2022 19:47
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

@DarshitChanpura
Copy link
Member Author

The latest commit e10c925 on main experienced same test failure.

org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

peternied
peternied approved these changes Nov 10, 2022
View reviewed changes
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Lets merge this so we can build a Rest API action to create a user end to end. Flip it to draft and we can see about merging

@DarshitChanpura DarshitChanpura marked this pull request as ready for review November 14, 2022 19:21
@DarshitChanpura
Copy link
Member Author

We can merge this. I'll create a follow-up PR to add rest API for createUser functionality

@peternied peternied merged commit 01f7a50 into opensearch-project:feature/identity Nov 14, 2022
@DarshitChanpura DarshitChanpura mentioned this pull request Jan 11, 2023
Open
35 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peternied peternied peternied approved these changes

@reta reta Awaiting requested review from reta reta is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DarshitChanpura @codecov-commenter @peternied @cwperks