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

IGNITE-9560 #4922

Merged
merged 135 commits into from Apr 26, 2019
Merged

IGNITE-9560 #4922

merged 135 commits into from Apr 26, 2019

Conversation

dgarus
Copy link
Contributor

@dgarus dgarus commented Oct 8, 2018

No description provided.

Copy link
Contributor

@yzhdanov yzhdanov left a comment

Choose a reason for hiding this comment

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

Guys,

I have looked through the PR and left couple of comments through the code. Please take a look.

The biggest issue for me was to understand logic behind classes hierarchy and relationships between IgniteSecurity, GridSecurityProcessor, IgniteSecurityProcessor, etc. It is still unclear for me. I would appreciate if someone creates a wiki page with design description. Then I take a look once again.

One more question - do we really need "withContext()" notation? Is that true that we never need to call withContext() several times wrapping and unwrapping the context multiple times? If yes and we do it only once on message processing start then logic behind wrapping and unwrapping can be simplified a lot. Do you agree?

Also note that adding 16 bytes field to IO message may affect performance for cases when security is off. Please suggest another solution.

There are also several comments from Nikolay Izhikov still unresolved. Please address all of them.

Please also review English in javadocs.

/**
* Default Grid security Manager implementation.
*/
public class IgniteSecurityProcessor implements IgniteSecurity, GridProcessor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not extending ProcessorAdapter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IgniteSecurityProcessor delegates all methods of GridProcessor to inner GridSecurityProcessor.
Adapter as a parent usage may lead to a situation when some delegation broke (no-op implementation from adapter used).

@dgarus
Copy link
Contributor Author

dgarus commented Apr 17, 2019

@yzhdanov hello!
Answers for questions:

  1. The biggest issue for me was to understand logic behind classes hierarchy and
    relationships between IgniteSecurity, GridSecurityProcessor, IgniteSecurityProcessor, etc.

Answer:
IgniteSecurity is an interface that AI is used to authorize operations.
Developers can obtain IgniteSecurity through GridKernalContext#security method.

There are two implementations of IgniteSecurity:

  • NoOpIgniteSecurityProcessor - is used when security is in disabled mode.
  • IgniteSecurityProcessor - is used when security is enabled.
    IgniteSecurityProcessor gets an instance of GridSecurityProcessor while constructing and delegate it performing of authorizing operations.

Which GridSecurityProcessor should be used is defined through plugin as it was earlier.

  1. do we really need "withContext()" notation?
    Is that true that we never need to call withContext() several times wrapping and unwrapping the context multiple times?

Answer: That's not true, regular case is to start processing with the node's context,
then switch to request's context or listener creator's context, and then restore the node's context.
Seems, also possible case is to call withContext() inside another withContext() when you have user1's task causes user2's task,
but, currently we had no such feature at Ignite as far as I know.

@anton-vinogradov anton-vinogradov merged commit 64d3f00 into apache:master Apr 26, 2019
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.

None yet

5 participants