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

[WFCORE-5691] proposal for bearer token timeout introspection #578

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

Conversation

rsearls
Copy link

@rsearls rsearls commented Jun 6, 2024

@rsearls rsearls force-pushed the EAP7-1856-Bearer-Token-Authorization-Timeout branch from c351ece to 1de0fea Compare June 6, 2024 15:31
@bstansberry
Copy link
Contributor

Hi @rsearls. Please update the commit message, PR title and the 'Issue' section to focus on WFCORE-5691. These are upstream analyses so an EAP7 is at most a 'related issue' of interest to EAP developers who have the ability to view that project. Thanks!

@@ -0,0 +1,120 @@
= [Preview] Bearer token timeout configurability will be added to WildFly's Elytron subsystem.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to category front matter -- see the first seven lines of https://raw.githubusercontent.com/wildfly/wildfly-proposals/main/design-doc-template.adoc.

Copy link
Author

Choose a reason for hiding this comment

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

done


=== Hard Requirements

* Two new attributes, `connectionTimeout` and `readTimeout` will be
Copy link
Contributor

Choose a reason for hiding this comment

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

WildFly management API attributes should not be camel case, so these would be connection-timeout and read-timeout.

Copy link
Author

Choose a reason for hiding this comment

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

done

@rsearls rsearls force-pushed the EAP7-1856-Bearer-Token-Authorization-Timeout branch from 1260054 to 07162cb Compare June 6, 2024 18:23
Bearer Token Authorization is the process of authorizing HTTP requests based on
the existence and validity of a bearer token. The token carries within it
an expiration timestamp. The two parameters being added, connection-timeout
and read-timeout are placed on the URL used in retrieving the public key.
Copy link
Contributor

@fjuma fjuma Jun 12, 2024

Choose a reason for hiding this comment

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

"are placed on the URL" -> It would be good to reword this last sentence a bit to explain what the attributes will be used for, i.e., used when obtaining the public key from the OAuth2 provider.

Copy link
Author

Choose a reason for hiding this comment

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

done


* https://issues.redhat.com/browse/EAP7-1856[EAP7-1856]
* https://issues.redhat.com/browse/ELY-2189[ELY-2189]
* https://issues.redhat.com/browse/EAPSUP-640[EAPSUP-640]
Copy link
Contributor

@fjuma fjuma Jun 12, 2024

Choose a reason for hiding this comment

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

We can remove the reference to the EAPSUP I think, it should be linked from the EAP7 issue.

Copy link
Author

Choose a reason for hiding this comment

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

done


== Backwards Compatibility

For backward compatibility the default value of 2000 milliseconds will be used. This is a hard coded value used in Elytron since 2021.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check, is this default value used for both attributes?

Copy link
Author

Choose a reason for hiding this comment

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

yes. text added


== Test Plan

* WildFly Tests: Integration test cases implemented for functionality.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also have tests in the Elytron testsuite that make use of the read timeout and connection timeout?

Copy link
Author

Choose a reason for hiding this comment

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

line added

@fjuma
Copy link
Contributor

fjuma commented Jun 12, 2024

@rsearls Thanks for updating the commit message! Please also update the title as well to reference the community issue.

@rsearls
Copy link
Author

rsearls commented Jun 12, 2024

can you be more specific about what you want added to the title?

@fjuma
Copy link
Contributor

fjuma commented Jun 12, 2024

can you be more specific about what you want added to the title?

Instead of [EAP7-1856], it should reference the community issue, [WFCORE-5691] (just like the commit message was updated). Just to clarify, I'm referring to the PR title that appears at the top (not the analysis doc title). You should see an Edit button at the top of the PR near "[EAP7-1856] proposal for bearer token timeout introspection #578".

@rsearls rsearls force-pushed the EAP7-1856-Bearer-Token-Authorization-Timeout branch from 07162cb to 0624111 Compare June 12, 2024 14:54
@rsearls rsearls changed the title [EAP7-1856] proposal for bearer token timeout introspection [WFCORE-5691] proposal for bearer token timeout introspection Jun 13, 2024
Bearer Token Authorization is the process of authorizing HTTP requests based on
the existence and validity of a bearer token. The token carries within it
an expiration timestamp. The two parameters being added, connection-timeout
and read-timeout are placed on the URL when obtaining the public key from the OAuth2 provider.
Copy link
Contributor

Choose a reason for hiding this comment

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

What public key?

What does placing these parameters on the URL do?

A user story would be good here.

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

Successfully merging this pull request may close these issues.

3 participants