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

Move isExpired from SimpleJWS to JWS #91

Closed
wants to merge 1 commit into from
Closed

Move isExpired from SimpleJWS to JWS #91

wants to merge 1 commit into from

Conversation

chalasr
Copy link
Contributor

@chalasr chalasr commented Apr 10, 2016

I propose this change in order to be able to check if a JWS created from an instance of Namshi\JOSE\JWS is expired or not (and not only from SimpleJWS), sort as we can do the check even if using the SecLib encryption engine.

Or is there something in the method that is not directly compatible with the JWS class, or that doesn't make sense to exist into?
ATM I need to duplicate the isExpired method in my project to do this chek on Namshi\JOSE\JWS instances.

@odino
Copy link
Contributor

odino commented Apr 11, 2016

hey @chalasr, thanks for this :) why not using the SimpleJWS class rather than JWS? We created SimpleJWS to add convenient methods on top of the bare JWS implementation, that was the original idea on why those methods were there.

@chalasr
Copy link
Contributor Author

chalasr commented Apr 11, 2016

Hi @odino
In fact, the SimpleJWS class seems to be only for OpenSSL, rightt?
Because SimpleJWS doesn't take an encryption engine as constructor arg.
I need to use the SecLib and your doc sounds like that we have to use the JWS class.
So maybe, rather than move the method, I can make the SimpleJWS class working for SecLib?

@odino
Copy link
Contributor

odino commented Apr 11, 2016

@chalasr definitely! Im not sure why it shouldnt be working with seclib but I think the best solution here is to make it working regardless of the underlying sec implementation

Closing this for now, waiting for a PR :)

Thanks man!

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.

None yet

2 participants