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

Allow exp claim to have string value #3202

Closed
MohamedSabthar opened this issue Aug 3, 2022 · 18 comments
Closed

Allow exp claim to have string value #3202

MohamedSabthar opened this issue Aug 3, 2022 · 18 comments
Assignees
Labels

Comments

@MohamedSabthar
Copy link
Member

MohamedSabthar commented Aug 3, 2022

Description:
The current implementation of OAuth2 caches the Introspection response if there is an integer value presented for the exp field in the Introspection response and invalidates the cache entry when the token expires.

Describe your problem(s)
If the exp value is not integer value then the response cached for a default time period provided via IntrospectionServerConfig (.ie defaultTokenExpTimeInSeconds field).

Describe your solution(s)
Rather than just setting the expiration to default when it is not Integer we could check whether we can parse the incoming string to Integer and if it is a valid time, we can set it as the expiration, otherwise set it to the default expiration time.

Related Issues (optional):

Suggested Labels (optional):

Suggested Assignees (optional):

@MohamedSabthar MohamedSabthar added Type/Improvement module/oauth2 Team/PCM Protocol connector packages related issues labels Aug 3, 2022
@MohamedSabthar MohamedSabthar self-assigned this Aug 3, 2022
@MohamedSabthar MohamedSabthar added the Good first issue Good for newcomers label Sep 19, 2023
@ShatilKhan
Copy link

Hi @MohamedSabthar
I'd like to work on this issue as I see this is a Good First Issue
Can you give some guidance on which specific files to work on?

@MohamedSabthar
Copy link
Member Author

👋 Welcome, @ShatilKhan! 🚀

We're thrilled to have you join the Ballerina Lang community! Whether you're a seasoned developer or just starting your journey with Ballerina, we value your contributions and look forward to collaborating with you.
To help you get started, here are some essential resources:

  1. Understanding Ballerina Platform:
    Learn what Ballerina is all about: Ballerina Platform
  2. How to Contribute:
    Read our contributing guidelines to understand how you can contribute effectively: Contribution Guide
  3. Contributing to the Ballerina library:
    If you're interested in contributing to our library, here are the guidelines: Library Contribution Guide
  4. Learn Ballerina:
    If you're new to Ballerina or want to enhance your skills, our official website offers a wealth of learning resources: Learn Ballerina
  5. Get Help and Connect:
    Join our Discord community to chat with fellow Ballerina enthusiasts and get assistance: Ballerina Discord

Remember, no contribution is too small, and your feedback is invaluable. Feel free to ask questions, propose ideas, or report issues. Together, we can make Ballerina even better!
Happy coding! 🎉

@MohamedSabthar
Copy link
Member Author

MohamedSabthar commented Oct 13, 2023

@ShatilKhan , please review the codebase of the ballerina/oauth2 library, with a specific focus on the listener_oauth2_provider.bal file. Your primary task may involve making modifications to the prepareIntrospectionResponse, addToCache methods, and the IntrospectionResponse type to implement the proposed solution.

@ShatilKhan
Copy link

Thanks! I'm working on it

@ShatilKhan
Copy link

ShatilKhan commented Oct 17, 2023

A few questions @MohamedSabthar
I'm new to Ballerina lang, I have gotten some idea as how to implement it, it should be in the if else statements on the methods you mentioned mostly
Now I looked at the Ballerina Lang docs
Can I use AND / OR operators in if statements?
I didn't see it in the docs
My idea for implementation:
Write an if-else statement to parse the string , then check if it is int & valid time (not sure how I'll do this part yet)
After that if it's valid, we set defaultTokenExpTime

@MohamedSabthar
Copy link
Member Author

@ShatilKhan yes you can use logical operators in if statements. +1 for the idea.

@ShatilKhan
Copy link

I've raised an initial PR, will discuss some issues I'm facing there

@MohamedSabthar
Copy link
Member Author

I've raised an initial PR, will discuss some issues I'm facing there

Certainly, I'd be happy to help. Could you please provide more details about the issues you're facing with your initial PR?

@keizer619
Copy link
Member

@ShatilKhan Are you still working on this?

@ovindu-a
Copy link

ovindu-a commented Oct 1, 2024

Hi @MohamedSabthar, @keizer619 I would like to work on this issue.

@keizer619
Copy link
Member

Hi @ovindu-a

To help you get started, here are some essential resources:

  1. Understanding Ballerina Platform:
    Learn what Ballerina is all about: Ballerina Platform
  2. How to Contribute:
    Read our contributing guidelines to understand how you can contribute effectively: Contribution Guide
  3. Contributing to the Ballerina library:
    If you're interested in contributing to our library, here are the guidelines: Library Contribution Guide
  4. Learn Ballerina:
    If you're new to Ballerina or want to enhance your skills, our official website offers a wealth of learning resources: Learn Ballerina
  5. Get Help and Connect:
    Ask technical questions on Stack Overflow with with Ballerina tag and join our Discord community Ballerina Discord

@ovindu-a
Copy link

ovindu-a commented Oct 2, 2024

Thank you @keizer619, I will have a look and start working

@ovindu-a
Copy link

ovindu-a commented Oct 2, 2024

@keizer619 I was going through the code in the listener_oauth2_provider.bal file. I noticed that if the exp value is passed in as a string in the json response, the below typecast would give an error. Shall I edit the code below in order to accept the exp value as a string?

I believe this should solve the issue as exp isn't changed anywhere else. Please let me know if I am missing anything.

EXP => { introspectionResponse.exp = <int>payloadMap[key]; }

@MohamedSabthar
Copy link
Member Author

@keizer619 I was going through the code in the listener_oauth2_provider.bal file. I noticed that if the exp value is passed in as a string in the json response, the below typecast would give an error. Shall I edit the code below in order to accept the exp value as a string?

I believe this should solve the issue as exp isn't changed anywhere else. Please let me know if I am missing anything.

EXP => { introspectionResponse.exp = <int>payloadMap[key]; }

Hi @ovindu-a,
Yes, this is the right place for the change. However, instead of changing the exp value type to a string, you should handle the following:
If payloadMap[EXP] is

  1. already an integer, assign it directly to introspectionResponse.exp.
  2. a string, try to parse the string into an integer and then set the introspectionResponse.exp value.
  3. any other types or if there's a parsing error, you should set introspectionResponse.exp to a nil value.
    Additionally, in the case of a parsing error, make sure to log the error.

@ovindu-a
Copy link

ovindu-a commented Oct 2, 2024

Ohh right that makes sense. I will implement it. Thanks

@ovindu-a
Copy link

ovindu-a commented Oct 2, 2024

@MohamedSabthar I have made an initial commit. Please let me know if there are any changes to be made

@MohamedSabthar
Copy link
Member Author

@MohamedSabthar I have made an initial commit. Please let me know if there are any changes to be made

@ovindu-a, I've added a few comments. Please review and address them.

@MohamedSabthar
Copy link
Member Author

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

No branches or pull requests

5 participants