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.

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

[Extensions] Add a internal user provider interface #2713

Conversation

stephen-crawford
Copy link
Contributor

Description

This PR introduces an internal user provider interface and a security internal user provider which implements this interface. The purpose of this interface is to let us build the plugin-side handler for the extension installation listener in core. Once this PR is merged, we can make changes in core to add the actual interface there that the provider will implement. That will then let us listen to core for extension registration events.

Issues Resolved

This PR is 1/2 of the solution to #2667.

Testing

Adds isolated tests for the provider functionality.

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

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.


String authToken = "";
try {
if (request.uri().contains("/internalusers/" + username + "/authtoken")) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should create a separate handler for this API? It may get confusing that the same handler is used for multiple endpoints.

Can you also add more description to the PR about the API being introduced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The InternalUsersApiAction already includes several endpoints so I thought it was appropriate to add this here. It is just a special instance of the GET functionality.

I will add some extra documentation.

Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
@stephen-crawford stephen-crawford force-pushed the ServiceListenerInterface branch from 10dc5fe to b47a802 Compare May 1, 2023 15:39
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.

The interface is vague, can we tighten in the use case for extensions with service accounts?

*/
public interface InternalUserProvider {

public void putInternalUser(String userInfo) throws java.io.IOException;
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend against throwing IOException, can we try /catch where this is coming from or if you think its needed throw specific exceptions?

* This interface will be defined in core following these changes. For now this is used for testing in the Security Plugin.
*
*/
public interface InternalUserProvider {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making this a 'ServiceAccountManager'?

Then it would have only the use cases needed in OpenSearch core, during extensions registration for find or create with enable / disable?


public void putInternalUser(String userInfo) throws java.io.IOException;

public SecurityDynamicConfiguration<?> getInternalUser(String userInfo);
Copy link
Member

Choose a reason for hiding this comment

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

SecurityDynamicConfiguration shouldn't be part of the interface, we should return a public interface/class instead


public void removeInternalUser(String userInfo);

public String getInternalUserAuthToken(String userInfo) throws IOException;
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 this return type should be a string in OpenSearch, it should be some kind of interface around the auth token. @cwperks Did you have ideas about how we would pass refresh token in the java ecosystem?

@stephen-crawford
Copy link
Contributor Author

Hi @peternied, sorry for the late reply. This is more of a testing area than anything else at the moment. I appreciate you leaving early feedback though. I will certainly address your concerns once I start narrowing down on this work.

@stephen-crawford stephen-crawford deleted the ServiceListenerInterface branch December 21, 2023 15:09
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.

3 participants