-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Introduce Application Scopes for Extension Management #7850
Closed
stephen-crawford
wants to merge
96
commits into
opensearch-project:main
from
stephen-crawford:limitedScopes
Closed
Changes from all commits
Commits
Show all changes
96 commits
Select commit
Hold shift + click to select a range
493e6fd
Initial thoughts
peternied 32334f4
Compilable
peternied 069face
Basic scope checkpoints, need followup on how checks are performed
peternied 246177c
Plumbing for scope checking
peternied 5f08318
Example of protection for an extension point
peternied 5257d8a
More scoping coverage
peternied 21e2e48
Remove how extensions identiity is related to scope access
peternied 089e19a
Match main
stephen-crawford 7eadf07
Assign all action scopes
stephen-crawford 6b44985
Fix build
stephen-crawford 2c88559
fix test failures
stephen-crawford 23b923d
add basic scope tests
stephen-crawford 06d57d5
spotless
stephen-crawford dd4a093
Tests pass
stephen-crawford 52b4640
Update actions
stephen-crawford 856eb01
Application aware subject to encapsulate scope checks inside of OpenS…
peternied 44cffb3
Update scopes
stephen-crawford 635c9b5
update actions
stephen-crawford 72393fe
fix painless
stephen-crawford 402b443
fix change
stephen-crawford 8ac10db
Fix updates
stephen-crawford 5906c1c
Fix action swaps
stephen-crawford 6db77bd
fix repsonse
stephen-crawford 70c1960
fix repsonse
stephen-crawford e46a14b
fix repsonse
stephen-crawford 37e3da2
fix repsonse
stephen-crawford 38e0739
fix repsonse
stephen-crawford b16ee2c
reset actions
stephen-crawford 1570c32
reset actions
stephen-crawford e05ee6b
initial fixes
stephen-crawford d13cbe8
Update scope definitions
stephen-crawford 3cc021b
optimize
stephen-crawford 8efb62b
Add ApplicationService
stephen-crawford 8beb430
Update scopes
stephen-crawford a1326f3
Fix precommit
stephen-crawford 6f63f6e
Add javadocs
stephen-crawford b860705
add pacakge info
stephen-crawford d41c52a
Pass tests
stephen-crawford 779a43e
remove debug
stephen-crawford 60465e5
Update shiro
stephen-crawford 8f5539a
Update CHANGELOG.md
stephen-crawford c1c3d28
Update Scopes
stephen-crawford 836b564
Refactor
stephen-crawford 05282af
Update extension manager
stephen-crawford 317ff2a
Finish action scopes refactor
stephen-crawford 4bd34e2
Add outline of extension point test
stephen-crawford 34a4b00
Remove extra exception
stephen-crawford 131f6b7
ActionScopetests pass
stephen-crawford 443ff0c
Pas all tests
stephen-crawford 600385e
Fix exception tests
stephen-crawford bf03fc4
Merge branch 'opensearch-project:main' into limitedScopes
stephen-crawford 0c380ac
Add noop applicationManager
stephen-crawford 2bb390a
remove user scopes
stephen-crawford c24f75b
add message
stephen-crawford bae552b
Apply suggestions from code review
stephen-crawford 926cb0c
Apply suggestions from code review
stephen-crawford 556877c
fix rebase
stephen-crawford 90291e6
Merge branch 'main' into limitedScopes
stephen-crawford e89239f
spotless
stephen-crawford 7a776b6
Fix exception issue
stephen-crawford 78f0246
Merge branch 'main' into limitedScopes
stephen-crawford 0d1f730
Fix imports after rebase
stephen-crawford a689cb1
fix failure
stephen-crawford 4431867
Merge branch 'main' into limitedScopes
stephen-crawford 76aceca
Peter's feedback
stephen-crawford 8143851
Merge branch 'opensearch-project:main' into limitedScopes
stephen-crawford 54fe5e7
Merge branch 'opensearch-project:main' into limitedScopes
stephen-crawford fcf319a
Update code coverage
stephen-crawford 2928b75
Merge branch 'main' into limitedScopes
stephen-crawford c74ab97
Update imports
stephen-crawford 91cf161
Merge branch 'main' into limitedScopes
stephen-crawford 3502517
just need to add extension point test
stephen-crawford 4f942d3
Add test
stephen-crawford c3fdaff
spotless
stephen-crawford 904f034
Update plugins/identity-shiro/src/main/java/org/opensearch/identity/s…
stephen-crawford ca5ef1d
Update server/src/main/java/org/opensearch/cluster/node/DiscoveryNode…
stephen-crawford 7e94469
Update to support writeables
stephen-crawford 67d6251
Fix initialization
stephen-crawford 6b7bfec
Update usage
stephen-crawford 2e5cc0f
Fix package order
stephen-crawford 3d7d780
Update server/src/main/java/org/opensearch/extensions/DiscoveryExtens…
stephen-crawford ef949f4
Update server/src/main/java/org/opensearch/extensions/DiscoveryExtens…
stephen-crawford 7e77422
Fix renames
stephen-crawford 3f4c491
Spotless
stephen-crawford 7be9930
Update xcontext
stephen-crawford a0acf53
Fix test
stephen-crawford 498e245
Update writeable and xcontent use
stephen-crawford 787e1f7
Fix style
stephen-crawford 0c868bd
fix streaming
stephen-crawford d8578f7
Merge remote-tracking branch 'origin/main' into limitedScopes
peternied 49d5290
Clean up subject creep
peternied aeaa324
Resolve shiro test issue
peternied 1c8dbe5
IT placeholders, refactor to support assuming identities
peternied 027b9e7
Merge branch 'main' into limitedScopes
stephen-crawford e925550
Add more details the integration test
peternied 756954b
Merge branch 'opensearch-project:main' into limitedScopes
stephen-crawford File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
74 changes: 0 additions & 74 deletions
74
plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroSubjectTests.java
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
81 changes: 81 additions & 0 deletions
81
server/src/internalClusterTest/java/org/opensearch/identity/IdentityServiceIT.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
*/ | ||
|
||
package org.opensearch.identity; | ||
|
||
import org.apache.lucene.index.LeafReaderContext; | ||
import org.apache.lucene.search.CollectionTerminatedException; | ||
import org.apache.lucene.search.ScoreMode; | ||
|
||
import org.opensearch.ExceptionsHelper; | ||
import org.opensearch.action.ActionListener; | ||
import org.opensearch.action.admin.cluster.node.stats.NodeStats; | ||
import org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest; | ||
import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; | ||
import org.opensearch.action.index.IndexRequest; | ||
import org.opensearch.action.index.IndexResponse; | ||
import org.opensearch.action.support.IndicesOptions; | ||
import org.opensearch.action.support.WriteRequest; | ||
import org.opensearch.client.Client; | ||
import org.opensearch.cluster.metadata.IndexMetadata; | ||
import org.opensearch.common.breaker.CircuitBreaker; | ||
import org.opensearch.core.common.io.stream.StreamInput; | ||
import org.opensearch.core.common.io.stream.StreamOutput; | ||
import org.opensearch.common.settings.Settings; | ||
import org.opensearch.common.unit.TimeValue; | ||
import org.opensearch.common.util.concurrent.AtomicArray; | ||
import org.opensearch.core.common.Strings; | ||
import org.opensearch.core.xcontent.ObjectParser; | ||
import org.opensearch.core.xcontent.XContentBuilder; | ||
import org.opensearch.index.IndexSettings; | ||
import org.opensearch.index.query.QueryShardContext; | ||
import org.opensearch.index.query.RangeQueryBuilder; | ||
import org.opensearch.index.shard.IndexShard; | ||
import org.opensearch.indices.IndicesService; | ||
import org.opensearch.indices.replication.common.ReplicationType; | ||
import org.opensearch.plugins.Plugin; | ||
import org.opensearch.plugins.SearchPlugin; | ||
import org.opensearch.core.rest.RestStatus; | ||
import org.opensearch.search.DocValueFormat; | ||
import org.opensearch.search.SearchHit; | ||
import org.opensearch.search.aggregations.AbstractAggregationBuilder; | ||
import org.opensearch.search.aggregations.AggregationBuilder; | ||
import org.opensearch.search.aggregations.Aggregations; | ||
import org.opensearch.search.aggregations.Aggregator; | ||
import org.opensearch.search.aggregations.AggregatorBase; | ||
import org.opensearch.search.aggregations.AggregatorFactories; | ||
import org.opensearch.search.aggregations.AggregatorFactory; | ||
import org.opensearch.search.aggregations.CardinalityUpperBound; | ||
import org.opensearch.search.aggregations.InternalAggregation; | ||
import org.opensearch.search.aggregations.LeafBucketCollector; | ||
import org.opensearch.search.aggregations.bucket.terms.LongTerms; | ||
import org.opensearch.search.aggregations.bucket.terms.TermsAggregationBuilder; | ||
import org.opensearch.search.aggregations.metrics.InternalMax; | ||
import org.opensearch.search.aggregations.support.ValueType; | ||
import org.opensearch.search.builder.SearchSourceBuilder; | ||
import org.opensearch.search.fetch.FetchSubPhase; | ||
import org.opensearch.search.fetch.FetchSubPhaseProcessor; | ||
import org.opensearch.search.internal.SearchContext; | ||
import org.opensearch.test.OpenSearchIntegTestCase; | ||
|
||
import java.io.IOException; | ||
import java.util.Collection; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.concurrent.CountDownLatch; | ||
|
||
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; | ||
import static org.hamcrest.Matchers.containsString; | ||
import static org.hamcrest.Matchers.equalTo; | ||
|
||
public class IdentityServiceIT extends OpenSearchIntegTestCase { | ||
|
||
// TODO: Verify that application aware subject is persisted on the same request | ||
|
||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not mix application and user subject, it should either one or another, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is included because it is an optional defined in the Subject interface. The idea is that we are able to differentiate between human users and applications by checking if a subject has an associated application inside the application manager.
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean this should be designed as exclusive abstractions, the optional just branch off the different states and makes API prone to misuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a Subject there the notion of a principal. In different frameworks it is represented as, an ordered collections of principals, other have multiple principals with one called a primary, or in what is proposed here there is a user principle and a separate application principal. What do you think?
I like the approach to have a distinct application principal as OpenSearch application concept for plugin and extension systems is limited to core. The Application manager that @scrawfor99 has proposed isn't meant to be accessed/extended by extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see that in the current design: every single (meaningful) usage of the
getApplication()
delegates togetPrincipal()
(see https://github.com/opensearch-project/OpenSearch/pull/7850/files#diff-1aa331c528b9d022fdd78cc985d90857b10cf9d0d5da28bafd2acb5dbbf799e3R166 fe), there is no primary or secondary. This is why I think we have to split those into distinct subjects: user, application (may be later plugin or extension).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've revisited many of these usages and largely removed subject's usage. There is another pull request [link] where we are adding service accounts which are tied to applications, and allow them to make REST requests - IMO this is a better personification as it allows applications to follow the same external authentication processes as users.
Contrived example of how the subject is changable over time;
So if I make a request with my openSearch client, after its authenticated, its
{sub: peternied}
, and then it flows into a plugins action handler, it should be{sub: peternied, app: machine_learning}
, then after the request leaves the handler, it becomes just{sub: peternied}
, if it enters another plugin entry point{sub: peternied, app: security_plugin}
gets changed. The primary subject is still me; however, the application changes as the request is handled in OpenSearch.peternied, machine_learning, security_plugin are all principals, but you cannot authenticate with the cluster as machine_learning as it isn't a Subject. Let me know if this helps clarify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, thanks @peternied , it cleared the confusion for me, so this is basically an originator, right? Should we call it as such?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good pull on originator [1] is the correct name. I think on the subject, we would switch to getOriginator() as you suggest, but keep the ApplicationAwareSubject which handle when the originator is an plugin/extension.