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

Add @Transient to OAuth2IntrospectionAuthenticationToken #6841

Closed
wants to merge 6 commits into from
Closed

Add @Transient to OAuth2IntrospectionAuthenticationToken #6841

wants to merge 6 commits into from

Conversation

huytn12
Copy link

@huytn12 huytn12 commented May 4, 2019

No description provided.

@jzheaux
Copy link
Contributor

jzheaux commented May 5, 2019

@huytn12 Thanks for the update! I have two questions:

  • Are you taking a look at why the build is failing?
  • Will you please also squash your commits into one commit, in preparation for merging?

Of course, let me know if you'd like help with either of these.

@huytn12
Copy link
Author

huytn12 commented May 7, 2019

@jzheaux Sorry for being delayed.
I looked at the build log and noticed that the build failed at

Task :spring-security-web:jacocoTestReport
[ant:jacocoReport] Classes in bundle 'spring-security-web' do no match with execution data. For report generation the same class files must be used as at runtime.
[ant:jacocoReport] Execution data for class org/springframework/security/web/debug/Logger does not match.

Task :spring-security-web:check
Task :spring-security-web:build

FAILURE: Build failed with an exception.

  • What went wrong:
    Execution failed for task ':spring-security-config:compileTestJava'.

Compilation failed; see the compiler error output for details.

So I believe there's something wrong when I added new test. I'm not so sure how to fix the issue so It would great if you could help me with it.
And yes, I will squash my commits into one.
Thanks

@jzheaux
Copy link
Contributor

jzheaux commented May 7, 2019

When I build your branch, the output shows this error:

/home/jzheaux/dev/spring-projects/spring-security/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java:571: error: cannot find symbol
        assertThat(result.getRequest().getSession(false)).isNull();

Does that give you enough to go off of?

@rwinch rwinch added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels May 8, 2019
@jzheaux
Copy link
Contributor

jzheaux commented May 8, 2019

Also, @huytn12, will you please update the title and description of this PR so it reflects the issue you are solving?

A title like "Add @transient to OAuth2IntrospectionAuthenticationToken" makes sense to me.

The description would link to the ticket at a minimum.

@huytn12 huytn12 changed the title Create a test for opaque token Add @transient to OAuth2IntrospectionAuthenticationToken May 8, 2019
@jzheaux jzheaux changed the title Add @transient to OAuth2IntrospectionAuthenticationToken Add @Transient to OAuth2IntrospectionAuthenticationToken May 8, 2019
@huytn12
Copy link
Author

huytn12 commented May 8, 2019

@jzheaux I updated the test and now the build failed again with different error. I look at the log file and found this

[ant:jacocoReport] Classes in bundle 'spring-security-web' do no match with execution data. For report generation the same class files must be used as at runtime.
[ant:jacocoReport] Execution data for class org/springframework/security/web/debug/Logger does not match.

I think the the execution data for class doesn't exist. I'm not so sure how to fix this issue. Could you give me some help?

@jzheaux
Copy link
Contributor

jzheaux commented May 8, 2019

When I compile your branch locally, I get:

/home/jzheaux/dev/spring-projects/spring-security/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java:569: error: incompatible types: ResultActions cannot be converted to MvcResult
                                .andExpect(content().string("test-subject"));

Are you able to run the build locally?

Are you using an IDE? It should be able to highlight these compiler errors for you before you push.

If not, then I recommend starting fresh. Get the library running in your IDE. That way, you'll be able to run the test yourself and see whether it passes or fails before pushing.

@rwinch rwinch added the status: waiting-for-feedback We need additional information before we can continue label May 13, 2019
@jzheaux
Copy link
Contributor

jzheaux commented May 14, 2019

@huytn12 is this something you are still interesting in contributing?

@florian42
Copy link
Contributor

@jzheaux if @huytn12 isn’t interested anymore I’d love to finish the test case and create a new pr.

@jzheaux
Copy link
Contributor

jzheaux commented May 20, 2019

@florian42 thank you, that'd be great. Please also link your PR to #6829, the related issue.

@jzheaux jzheaux closed this May 20, 2019
@jzheaux
Copy link
Contributor

jzheaux commented May 28, 2019

@florian42 is this still something you'd like to contribute a PR for?

@florian42
Copy link
Contributor

Hi yes, didn’t get a notification. Will do it tomorrow 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: waiting-for-feedback We need additional information before we can continue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants