You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "Miroslav Smiljanic (Created) (JIRA)" <ji...@apache.org> on 2011/10/19 14:32:11 UTC
[jira] [Created] (JCR-3120) improve implementation of
UserManagerImpl#getAuthorizable(NodeImpl n) and UserImporter#handlePropInfo
- debug log level is not sufficient
improve implementation of UserManagerImpl#getAuthorizable(NodeImpl n) and UserImporter#handlePropInfo - debug log level is not sufficient
-----------------------------------------------------------------------------------------------------------------------------------------
Key: JCR-3120
URL: https://issues.apache.org/jira/browse/JCR-3120
Project: Jackrabbit Content Repository
Issue Type: Improvement
Components: jackrabbit-core
Affects Versions: 2.3.1
Environment: any
Reporter: Miroslav Smiljanic
Priority: Minor
This is current implementation:
Authorizable getAuthorizable(NodeImpl n) throws RepositoryException {
Authorizable authorz = null;
if (n != null) {
String path = n.getPath();
if (n.isNodeType(NT_REP_USER) && Text.isDescendant(usersPath, path)) {
authorz = createUser(n);
} else if (n.isNodeType(NT_REP_GROUP) && Text.isDescendant(groupsPath, path)) {
authorz = createGroup(n);
} else {
/* else some other node type or outside of the valid user/group
hierarchy -> return null. */
log.debug("Unexpected user nodetype " + n.getPrimaryNodeType().getName());
}
} /* else no matching node -> return null */
return authorz;
}
It seems that 'else' branch can be improved, at least by increasing log level. But I think, that best way is to throw exception.
Current message can also be misleading, in case when user type is correct but check Text.isDescendant fails.
Above method is called from within UserImporter#handlePropInfo
...
Authorizable a = userManager.getAuthorizable(parent);
if (a == null) {
log.debug("Cannot handle protected PropInfo " + protectedPropInfo + ". Node " + parent + " doesn't represent a valid Authorizable.");
return false;
}
....
Here again log level is debug. Because at this point we have return statement, property 'principalName' is not set, and if we try to save session following exception will be thrown:
javax.jcr.nodetype.ConstraintViolationException: /home/public/users/b/bb2: mandatory property {internal}password does not exist
at org.apache.jackrabbit.core.ItemSaveOperation.validateTransientItems(ItemSaveOperation.java:537)
at org.apache.jackrabbit.core.ItemSaveOperation.perform(ItemSaveOperation.java:216)
at org.apache.jackrabbit.core.session.SessionState.perform(SessionState.java:200)
at org.apache.jackrabbit.core.ItemImpl.perform(ItemImpl.java:91)
at org.apache.jackrabbit.core.ItemImpl.save(ItemImpl.java:329)
...
So if the log level is not set to 'debug' it is not obvious why mentioned property is missing. Use case and root cause is that 'path' (/home/public/users/b/bb2) is not descendant of 'usersPath' (/home/users).
Regards,
Miroslav
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (JCR-3120) Change log level in
UserManagerImpl#getAuthorizable(NodeImpl) and UserImporter#handlePropInfo
Posted by "angela (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3120?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
angela updated JCR-3120:
------------------------
Component/s: security
Environment: (was: any)
Affects Version/s: 2.0
2.1
2.2
2.3
Assignee: angela
Summary: Change log level in UserManagerImpl#getAuthorizable(NodeImpl) and UserImporter#handlePropInfo (was: improve implementation of UserManagerImpl#getAuthorizable(NodeImpl n) and UserImporter#handlePropInfo - debug log level is not sufficient)
> Change log level in UserManagerImpl#getAuthorizable(NodeImpl) and UserImporter#handlePropInfo
> ---------------------------------------------------------------------------------------------
>
> Key: JCR-3120
> URL: https://issues.apache.org/jira/browse/JCR-3120
> Project: Jackrabbit Content Repository
> Issue Type: Improvement
> Components: jackrabbit-core, security
> Affects Versions: 2.0, 2.1, 2.2, 2.3, 2.3.1
> Reporter: Miroslav Smiljanic
> Assignee: angela
> Priority: Minor
> Labels: jackrabbit-core,
>
> This is current implementation:
> Authorizable getAuthorizable(NodeImpl n) throws RepositoryException {
> Authorizable authorz = null;
> if (n != null) {
> String path = n.getPath();
> if (n.isNodeType(NT_REP_USER) && Text.isDescendant(usersPath, path)) {
> authorz = createUser(n);
> } else if (n.isNodeType(NT_REP_GROUP) && Text.isDescendant(groupsPath, path)) {
> authorz = createGroup(n);
> } else {
> /* else some other node type or outside of the valid user/group
> hierarchy -> return null. */
> log.debug("Unexpected user nodetype " + n.getPrimaryNodeType().getName());
> }
> } /* else no matching node -> return null */
> return authorz;
> }
> It seems that 'else' branch can be improved, at least by increasing log level. But I think, that best way is to throw exception.
> Current message can also be misleading, in case when user type is correct but check Text.isDescendant fails.
> Above method is called from within UserImporter#handlePropInfo
> ...
> Authorizable a = userManager.getAuthorizable(parent);
> if (a == null) {
> log.debug("Cannot handle protected PropInfo " + protectedPropInfo + ". Node " + parent + " doesn't represent a valid Authorizable.");
> return false;
> }
> ....
> Here again log level is debug. Because at this point we have return statement, property 'principalName' is not set, and if we try to save session following exception will be thrown:
> javax.jcr.nodetype.ConstraintViolationException: /home/public/users/b/bb2: mandatory property {internal}password does not exist
> at org.apache.jackrabbit.core.ItemSaveOperation.validateTransientItems(ItemSaveOperation.java:537)
> at org.apache.jackrabbit.core.ItemSaveOperation.perform(ItemSaveOperation.java:216)
> at org.apache.jackrabbit.core.session.SessionState.perform(SessionState.java:200)
> at org.apache.jackrabbit.core.ItemImpl.perform(ItemImpl.java:91)
> at org.apache.jackrabbit.core.ItemImpl.save(ItemImpl.java:329)
> ...
>
> So if the log level is not set to 'debug' it is not obvious why mentioned property is missing. Use case and root cause is that 'path' (/home/public/users/b/bb2) is not descendant of 'usersPath' (/home/users).
> Regards,
> Miroslav
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (JCR-3120) Change log level in
UserManagerImpl#getAuthorizable(NodeImpl) and UserImporter#handlePropInfo
Posted by "Jukka Zitting (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3120?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jukka Zitting updated JCR-3120:
-------------------------------
Fix Version/s: (was: 2.4)
2.3.3
> Change log level in UserManagerImpl#getAuthorizable(NodeImpl) and UserImporter#handlePropInfo
> ---------------------------------------------------------------------------------------------
>
> Key: JCR-3120
> URL: https://issues.apache.org/jira/browse/JCR-3120
> Project: Jackrabbit Content Repository
> Issue Type: Improvement
> Components: jackrabbit-core, security
> Affects Versions: 2.0, 2.1, 2.2, 2.3, 2.3.1
> Reporter: Miroslav Smiljanic
> Assignee: angela
> Priority: Minor
> Labels: jackrabbit-core,
> Fix For: 2.3.3
>
>
> This is current implementation:
> Authorizable getAuthorizable(NodeImpl n) throws RepositoryException {
> Authorizable authorz = null;
> if (n != null) {
> String path = n.getPath();
> if (n.isNodeType(NT_REP_USER) && Text.isDescendant(usersPath, path)) {
> authorz = createUser(n);
> } else if (n.isNodeType(NT_REP_GROUP) && Text.isDescendant(groupsPath, path)) {
> authorz = createGroup(n);
> } else {
> /* else some other node type or outside of the valid user/group
> hierarchy -> return null. */
> log.debug("Unexpected user nodetype " + n.getPrimaryNodeType().getName());
> }
> } /* else no matching node -> return null */
> return authorz;
> }
> It seems that 'else' branch can be improved, at least by increasing log level. But I think, that best way is to throw exception.
> Current message can also be misleading, in case when user type is correct but check Text.isDescendant fails.
> Above method is called from within UserImporter#handlePropInfo
> ...
> Authorizable a = userManager.getAuthorizable(parent);
> if (a == null) {
> log.debug("Cannot handle protected PropInfo " + protectedPropInfo + ". Node " + parent + " doesn't represent a valid Authorizable.");
> return false;
> }
> ....
> Here again log level is debug. Because at this point we have return statement, property 'principalName' is not set, and if we try to save session following exception will be thrown:
> javax.jcr.nodetype.ConstraintViolationException: /home/public/users/b/bb2: mandatory property {internal}password does not exist
> at org.apache.jackrabbit.core.ItemSaveOperation.validateTransientItems(ItemSaveOperation.java:537)
> at org.apache.jackrabbit.core.ItemSaveOperation.perform(ItemSaveOperation.java:216)
> at org.apache.jackrabbit.core.session.SessionState.perform(SessionState.java:200)
> at org.apache.jackrabbit.core.ItemImpl.perform(ItemImpl.java:91)
> at org.apache.jackrabbit.core.ItemImpl.save(ItemImpl.java:329)
> ...
>
> So if the log level is not set to 'debug' it is not obvious why mentioned property is missing. Use case and root cause is that 'path' (/home/public/users/b/bb2) is not descendant of 'usersPath' (/home/users).
> Regards,
> Miroslav
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (JCR-3120) Change log level in
UserManagerImpl#getAuthorizable(NodeImpl) and UserImporter#handlePropInfo
Posted by "angela (Resolved) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3120?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
angela resolved JCR-3120.
-------------------------
Resolution: Fixed
Fix Version/s: 2.4
i rearranged the code in UserManager impl in order to have the log output be more
specific. in addition i increased the log level to warn and error respectively.
regarding throwing an exception: i decided not to do so for consistency with the
overall behavior of the user manager that is defined to return 'null' if no valid
user/group can be retrieved and only throws an exception if an unexpected error
occurs (retrieving path from node or node type name fails).
> Change log level in UserManagerImpl#getAuthorizable(NodeImpl) and UserImporter#handlePropInfo
> ---------------------------------------------------------------------------------------------
>
> Key: JCR-3120
> URL: https://issues.apache.org/jira/browse/JCR-3120
> Project: Jackrabbit Content Repository
> Issue Type: Improvement
> Components: jackrabbit-core, security
> Affects Versions: 2.0, 2.1, 2.2, 2.3, 2.3.1
> Reporter: Miroslav Smiljanic
> Assignee: angela
> Priority: Minor
> Labels: jackrabbit-core,
> Fix For: 2.4
>
>
> This is current implementation:
> Authorizable getAuthorizable(NodeImpl n) throws RepositoryException {
> Authorizable authorz = null;
> if (n != null) {
> String path = n.getPath();
> if (n.isNodeType(NT_REP_USER) && Text.isDescendant(usersPath, path)) {
> authorz = createUser(n);
> } else if (n.isNodeType(NT_REP_GROUP) && Text.isDescendant(groupsPath, path)) {
> authorz = createGroup(n);
> } else {
> /* else some other node type or outside of the valid user/group
> hierarchy -> return null. */
> log.debug("Unexpected user nodetype " + n.getPrimaryNodeType().getName());
> }
> } /* else no matching node -> return null */
> return authorz;
> }
> It seems that 'else' branch can be improved, at least by increasing log level. But I think, that best way is to throw exception.
> Current message can also be misleading, in case when user type is correct but check Text.isDescendant fails.
> Above method is called from within UserImporter#handlePropInfo
> ...
> Authorizable a = userManager.getAuthorizable(parent);
> if (a == null) {
> log.debug("Cannot handle protected PropInfo " + protectedPropInfo + ". Node " + parent + " doesn't represent a valid Authorizable.");
> return false;
> }
> ....
> Here again log level is debug. Because at this point we have return statement, property 'principalName' is not set, and if we try to save session following exception will be thrown:
> javax.jcr.nodetype.ConstraintViolationException: /home/public/users/b/bb2: mandatory property {internal}password does not exist
> at org.apache.jackrabbit.core.ItemSaveOperation.validateTransientItems(ItemSaveOperation.java:537)
> at org.apache.jackrabbit.core.ItemSaveOperation.perform(ItemSaveOperation.java:216)
> at org.apache.jackrabbit.core.session.SessionState.perform(SessionState.java:200)
> at org.apache.jackrabbit.core.ItemImpl.perform(ItemImpl.java:91)
> at org.apache.jackrabbit.core.ItemImpl.save(ItemImpl.java:329)
> ...
>
> So if the log level is not set to 'debug' it is not obvious why mentioned property is missing. Use case and root cause is that 'path' (/home/public/users/b/bb2) is not descendant of 'usersPath' (/home/users).
> Regards,
> Miroslav
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira