[bugfix] Fix for NPE in xquery.functions.securitymanager.IdFunction #4671
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
line-o
reviewed
Jan 9, 2023
| subjectToXml(builder, context.getEffectiveUser()); | ||
| builder.endElement(); | ||
| final Subject effectiveUser = context.getEffectiveUser(); | ||
| if (effectiveUser != null) { |
There was a problem hiding this comment.
I would prefer to combine the nested conditions to
if (effectiveUser != null && (
realUser == null || !sameUserWithSameGroups(realUser, effectiveUser)
)) {
There was a problem hiding this comment.
Additionally the method sameUserWithSameGroups could handle null values for both realUser and effectiveUser
There was a problem hiding this comment.
I thought about that, but to me the conditions mean different things, and I kept them separate.
Either way, the result, and maybe the bytecode, is exactly the same.
I also thought about checking for null inside sameUserWithSameGroups, which might make the intention of the code clearer.
There was a problem hiding this comment.
If both are null then it should return false true.
There was a problem hiding this comment.
This would simplify the condition to
if (!sameUserwithSameGroups(realUser, effectiveUser)) {...}
There was a problem hiding this comment.
:D I'll shut up until you have completed your review.
Nowadays, I prefer code that tells a story over concise code. I tend to take apart conditions like && ( || ) in my head when I read them.
But code that is robust against null (even though sameUserWithSameGroups is private) is even more important.
There was a problem hiding this comment.
I should have combined them into one instead of brain-dumping :)
line-o
reviewed
Jan 9, 2023
exist-core/src/main/java/org/exist/xquery/functions/securitymanager/IdFunction.java
Show resolved
Hide resolved
adamretter
changed the title
adamretter
changed the title
Bumps commons-io from 2.11.0 to 2.12.0. --- updated-dependencies: - dependency-name: commons-io:commons-io dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
…ns-io-commons-io-2.12.0 build(deps): bump commons-io from 2.11.0 to 2.12.0
…le-declaration Remove pre-1.0 syntax for variable declarations
It appears perfectly acceptable for UntypedAtomicValue.convertTo to convert to subtypes of Type.STRING as StringValue(… value, requiredType). The conversion was omitted, and the omission was breaking “castable as” for Type.NCNAME in particular. These conversions are already acceptable for StringValue(s), and are borrowed from there.
…stable-error [fix] castable as for untyped atomic value(s)
…-modules Set the default `enforce-index-use` to strict for all tests
Bumps [maven-scm-plugin](https://github.com/apache/maven-scm) from 2.0.0 to 2.0.1. - [Commits](apache/maven-scm@maven-scm-2.0.0...maven-scm-2.0.1) --- updated-dependencies: - dependency-name: org.apache.maven.plugins:maven-scm-plugin dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
…pache.maven.plugins-maven-scm-plugin-2.0.1 build(deps): bump maven-scm-plugin from 2.0.0 to 2.0.1
…with its dependencies. Closes eXist-db#4779 Closes Antibrumm/copy-maven-plugin#7
Bumps [docker-maven-plugin](https://github.com/fabric8io/docker-maven-plugin) from 0.42.1 to 0.43.0. - [Release notes](https://github.com/fabric8io/docker-maven-plugin/releases) - [Changelog](https://github.com/fabric8io/docker-maven-plugin/blob/master/doc/changelog.md) - [Commits](fabric8io/docker-maven-plugin@v0.42.1...v0.43.0) --- updated-dependencies: - dependency-name: io.fabric8:docker-maven-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
passing persistent nodes from XQuery to fn:transform as parameters doesn’t work. NodeProxy objects were not being considered; dereference their nodes and use the result appropriately.
Previously this resulted in org.exist.xquery.XPathException: java:java.lang.ClassCastException class org.exist.dom.persistent.NodeProxy cannot be cast to class org.w3c.dom.Node (org.exist.dom.persistent.NodeProxy is in unnamed module of loader 'app'; org.w3c.dom.Node is in module java.xml of loader 'bootstrap') Now it succeeds.
xmldb:exist: URIs were not being understood / resolved against correctly when looking for resources within the transformation code wrapping the Saxon transformer in our fn:transform implementation. Strictly they’re not valid URIs, which is what was the fundamental problem, so we need to add some special-case XMLDBURI handling. We also refactor just a little to pull the URI resolution code used by fn:transform into its own class in the fn:transform implementation package, and add unit tests for that extracted resolution.
…py-maven-plugin Use newer copy-maven-plugin which does not have CVE issues with its dependencies
Bumps [maven-source-plugin](https://github.com/apache/maven-source-plugin) from 3.2.1 to 3.3.0. - [Commits](apache/maven-source-plugin@maven-source-plugin-3.2.1...maven-source-plugin-3.3.0) --- updated-dependencies: - dependency-name: org.apache.maven.plugins:maven-source-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
just yield the relative URI unchanged, we were sticking the xmldb prefix in front.
Bumps [angus-mail](https://github.com/eclipse-ee4j/angus-mail) from 2.0.1 to 2.0.2. - [Release notes](https://github.com/eclipse-ee4j/angus-mail/releases) - [Commits](eclipse-ee4j/angus-mail@2.0.1...2.0.2) --- updated-dependencies: - dependency-name: org.eclipse.angus:angus-mail dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [maven-dependency-plugin](https://github.com/apache/maven-dependency-plugin) from 3.5.0 to 3.6.0. - [Commits](apache/maven-dependency-plugin@maven-dependency-plugin-3.5.0...maven-dependency-plugin-3.6.0) --- updated-dependencies: - dependency-name: org.apache.maven.plugins:maven-dependency-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
Tweak transformation’s CompileTimeURIResolver class to allow easier subclassing, to let unit test exercise it more easily.
…clipse.angus-angus-mail-2.0.2 build(deps): bump angus-mail from 2.0.1 to 2.0.2
Bumps [jakarta.mail-api](https://github.com/jakartaee/mail-api) from 2.1.1 to 2.1.2. - [Release notes](https://github.com/jakartaee/mail-api/releases) - [Commits](jakartaee/mail-api@2.1.1...2.1.2) --- updated-dependencies: - dependency-name: jakarta.mail:jakarta.mail-api dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
…ta.mail-jakarta.mail-api-2.1.2 build(deps): bump jakarta.mail-api from 2.1.1 to 2.1.2
inline experimental
…ath#append(NodePath)
…ath#addComponent(QName)
…, make sure to show the namespace when there is no prefix
…oString() was being called instead of QName#getStringValue()
… whereby QName#toString() was being called instead of QName#getStringValue()
Bumps org.apache.commons:commons-compress from 1.23.0 to 1.24.0. --- updated-dependencies: - dependency-name: org.apache.commons:commons-compress dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
…pache.commons-commons-compress-1.24.0 build(deps): bump org.apache.commons:commons-compress from 1.23.0 to 1.24.0
Signed-off-by: Patrick Reinhart <[email protected]>
Signed-off-by: Patrick Reinhart <[email protected]>
…llocation-strategy Fix a bug in Node Path equality
…cketed-element Fix issues with Lucene Index optimisations and Matches
[bugfix] increase the maven daemon timeout
Cleanup/configuration
Bumps [com.fasterxml.uuid:java-uuid-generator](https://github.com/cowtowncoder/java-uuid-generator) from 4.2.0 to 4.3.0. - [Commits](cowtowncoder/java-uuid-generator@java-uuid-generator-4.2.0...java-uuid-generator-4.3.0) --- updated-dependencies: - dependency-name: com.fasterxml.uuid:java-uuid-generator dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
…asterxml.uuid-java-uuid-generator-4.3.0 build(deps): bump com.fasterxml.uuid:java-uuid-generator from 4.2.0 to 4.3.0
|
@nverwer can you rebase this PR once again? |
Bumps [jakarta.xml.bind:jakarta.xml.bind-api](https://github.com/eclipse-ee4j/jaxb-api) from 4.0.0 to 4.0.1. - [Release notes](https://github.com/eclipse-ee4j/jaxb-api/releases) - [Commits](jakartaee/jaxb-api@4.0.0...4.0.1) --- updated-dependencies: - dependency-name: jakarta.xml.bind:jakarta.xml.bind-api dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
…ta.xml.bind-jakarta.xml.bind-api-4.0.1 build(deps-dev): bump jakarta.xml.bind:jakarta.xml.bind-api from 4.0.0 to 4.0.1
…into hotfix/4670-npe-in-IdFunction
|
@reinhapa I think that I rebased correctly. But I have to admit that I have become a bit lost in all the branches that seem relevant. |
|
@nverwer seems something is wrong. Did you rebase your changes on the current |
dizzzz
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This fixes a NPE in the
sm:id()function, that caused problems in one of my applications.Reference:
See #4670 .