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

Resolves #265 Pass in a HttpMethod to the call to hasAccessToWebResource(). #266

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

darranl
Copy link
Contributor

@darranl darranl commented Aug 18, 2022

No description provided.

@arjantijms arjantijms added this to the 3.1 milestone Aug 18, 2022
@arjantijms arjantijms added the TCK label Aug 18, 2022
Copy link
Contributor

@OndroMih OndroMih left a comment

Choose a reason for hiding this comment

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

We shouldn't change the test, the test seems completely valid to me as the API allows calling hasAccessToWebResource method without any methods. The only thing that's missing is clarifying the behavior in such case in the Javadoc, as I mentioned here: #262

@OndroMih
Copy link
Contributor

I'm curious what other committers think. @m0mus , @dblevins , @keilw , @rdebusscher , and others, what's your opinion?

@darranl
Copy link
Contributor Author

darranl commented Aug 18, 2022

@arjantijms
Copy link
Contributor

I think I remember what happened here. We had an overloaded method before; hasAccessToWebResource(java.lang.String), which specifically documented the 0 methods case. That one was merged into the one with the varargs, but then the javadoc wasn't updated.

The behaviour as it's now is technically correct and in the spirit of what was meant to be done (specifically what I meant to happen when I designed the method).

But, Darran is right that the javadoc is wrong. It indeed says "1 or more parameters", which is my mistake. Changing the test would help for WildFly to pass the TCK for Jakarta EE 10, but only if we can get an exception from the specification committee to update the TCK in a maintenance release.

Ondro is also right, that the target behaviour is as the test is now, and we would have to change the javadoc in the next release of Jakarta Security and then change the test back (or add another test for the 0 method case).

@darranl is it doable to simply update WildFly to accept the 0 method case as applying "to all the possible HTTP methods."? This would have to be done for the next release of Jakarta Security anyway, and as a bonus WildFly and GlassFish would be more compatible right now.

If WildFly would use WebResourcePermission as implementation detail (as Soteria is already doing by default), it would be trivial to support as well right? Jakarta Authorization has to be available and used by default in Jakarta EE, so there would be no reasons not to do that, would there be?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants