[
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: [email protected]
For additional commands, e-mail: [email protected]
