You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by "Alex Rudyy (JIRA)" <ji...@apache.org> on 2016/08/24 08:26:20 UTC
[jira] [Comment Edited] (QPID-7346) [Java Broker] Improve
Principals to record their origin
[ https://issues.apache.org/jira/browse/QPID-7346?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15433225#comment-15433225 ]
Alex Rudyy edited comment on QPID-7346 at 8/24/16 8:25 AM:
-----------------------------------------------------------
Lorenz,
Here are my review comments:
HashedUser#_authenticationProvider field is unused. I think we do not need this field.
PlainUser#_authenticationProvider field is unused. I think we do not need this field.
AbstractPasswordFilePrincipalDatabase
Field _authenticationProvider is unnecissary. It is only used to create instances of UsernamePrincipal, however, those principals are converted into PrincipalAdapter and AuthenticationProvider field is get ignored.
It seems the changes introduced in AbstractPasswordFilePrincipalDatabase are redundand as the existing code does not really need AuthenticationProvider.
I think that AbstractPasswordFilePrincipalDatabase should be changed in the following way:
Methods getUser should be changed to not return UsernamePrincipal but rather instance of Principal as required by interface
Taking that both PlainUser and HashedUser do not need AuthenticationProvider, the fields _authenticationProvider could be removed from PlainUser and HashedUser classes.
PrincipalDatabase#getAuthenticationProvider is removed
The principals created in AuthenticationProviderInitialiser and SASL callback handlers do not need AuthenticationProvider
The patch is attached
PrincipalDatabase
The implementations could easilly avoid calls to getAuthenticationProvider. If AbstractPasswordFilePrincipalDatabase is changed as suggested above, the method can be removed.
AbstractQueue
The change made to create exclusiveOwner might not work when owner attribute value does not follow the generic principal format.
Thus, it breakes backward compatibility with any existing solutions requesting queues with principal exclusivity where owner is not formated in generic way.
I think that GenericPrincipal should not be used here. Perhaps, code should fall back to UsernamePrincipal principal with null authentication provider if it fails to create GenericPrincipal.
If GenericPrincipal is required to use, I think, that we need to verify the existance of AuthenticationProvider having given type and name.
AnonymousAuthenticationProvider
I find it strange to distinquish anonymous users from different anonymous authentication providers. Having multiple anonymous authentication providers does not make sense but I am wondering whether current implementation would cause us problem if future.
was (Author: alex.rufous):
Lorenz,
I started reviewing of the changes you had made in this JIRA. Here are my review comments (in progress):
UsernamePrincipal#equals and UsernamePrincipal#hashCode do not take into account associated AuthenticationProvider
HashedUser#_authenticationProvider field is unused. I think we do not need this field.
PlainUser#_authenticationProvider field is unused. I think we do not need this field.
AbstractPasswordFilePrincipalDatabase
Field _authenticationProvider is unnecissary. It is only used to create instances of UsernamePrincipal, however, those principals are converted into PrincipalAdapter and AuthenticationProvider field is get ignored.
It seems the changes introduced in AbstractPasswordFilePrincipalDatabase are redundand as the existing code does not really need AuthenticationProvider.
I think that AbstractPasswordFilePrincipalDatabase should be changed in the following way:
Methods getUser should be changed to not return UsernamePrincipal but rather instance of Principal as required by interface
Taking that both PlainUser and HashedUser do not need AuthenticationProvider, the fields _authenticationProvider could be removed from PlainUser and HashedUser classes.
PrincipalDatabase#getAuthenticationProvider is removed
The principals created in AuthenticationProviderInitialiser and SASL callback handlers do not need AuthenticationProvider
The patch is attached
PrincipalDatabase
The implementations could easilly avoid calls to getAuthenticationProvider. If AbstractPasswordFilePrincipalDatabase is changed as suggested above, the method can be removed.
AbstractQueue
The change made to create exclusiveOwner might not work when owner attribute value does not follow the generic principal format.
Thus, it breakes backward compatibility with any existing solutions requesting queues with principal exclusivity where owner is not formated in generic way.
I think that GenericPrincipal should not be used here. Perhaps, code should fall back to UsernamePrincipal principal with null authentication provider if it fails to create GenericPrincipal.
If GenericPrincipal is required to use, I think, that we need to verify the existance of AuthenticationProvider having given type and name.
AnonymousAuthenticationProvider
I find it strange to distinquish anonymous users from different anonymous authentication providers. Having multiple anonymous authentication providers does not make sense but I am wondering whether current implementation would cause us problem if future.
> [Java Broker] Improve Principals to record their origin
> -------------------------------------------------------
>
> Key: QPID-7346
> URL: https://issues.apache.org/jira/browse/QPID-7346
> Project: Qpid
> Issue Type: Improvement
> Components: Java Broker
> Reporter: Lorenz Quack
> Fix For: qpid-java-6.1
>
> Attachments: 0001-QPID-7346-Java-Broker-WIP-Save-the-origin-of-a-Princ.patch, reverting-changes-for-principal-databases.diff
>
>
> Currently the broker uses a variety of different Principals (e.g., {{UsernamePrincipal}}, {{GroupPrincipal}}, ...).
> To make (de-)serialisation and future migration to a more sophisticate principal representation easier the principals should capture their origin (e.g., {{OAuth2AuthenticationProvider}}, {{FileBasedGroupProvider}}, ...).
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org