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

PASETO: Replace Payload struct by paseto.JSONToken + Improve helpers and public errors #5

Merged
merged 7 commits into from
May 28, 2021

Conversation

Jictyvoo
Copy link
Contributor

@Jictyvoo Jictyvoo commented May 21, 2021

As requested in #4 this PR improve some things about config and move all errors to helpers.go

  • All errors are now public
  • Improve default ErrorHandler error verification
  • Add ContextKey and TokenLookup configs to DefaultConfig

More additions

  • Replaced Payload struct by paseto.JSONToken struct
  • Fixed some typos
  • Use ConfigDefault values when a given config is empty, to be more consistent with ConfigDefault

- All errors are now public
- Improve default ErrorHandler error verification
- Add ContextKey and TokenLookup configs to DefaultConfig
@Jictyvoo Jictyvoo changed the title 🎇 Improve helpers PASETO: Improve helpers and public errors May 21, 2021
@ReneWerner87 ReneWerner87 linked an issue May 25, 2021 that may be closed by this pull request
@Jictyvoo
Copy link
Contributor Author

Jictyvoo commented May 28, 2021

I've update in another branch to replace the Payload struct and utilize the JSONToken in paseto module. @ReneWerner87 I add this update in this PR or create another one?

@ReneWerner87
Copy link
Member

here is the right place for the tests that are important for your customization, since the pull request is not closed yet, you should add them here

@Jictyvoo Jictyvoo changed the title PASETO: Improve helpers and public errors PASETO: Replace Payload struct by paseto.JSONToken + Improve helpers and public errors May 28, 2021
@Jictyvoo
Copy link
Contributor Author

Jictyvoo commented May 28, 2021

@ReneWerner87 I think it's done now.
Can you review the change of our custom Payload object to use the paseto.JSONToken?

Also, I'm thinking on use Storage to stores the uuid and check this ID in every token validation on this middleware, and to disable it, just don't pass any storage to the Config, what you think?

Of course, do this in another PR, cause is a big change

@ReneWerner87
Copy link
Member

ReneWerner87 commented May 28, 2021

@Jictyvoo hmm, you said you are through, however i don't see any new testcases for the errors ?

can you please include a test for these different errors where you check the correct misbehavior

some should already exist, some not yet

ErrExpiredToken  = errors.New("token has expired")
ErrMissingToken  = errors.New("missing PASETO token")
ErrDataUnmarshal = errors.New("can't unmarshal token data to Payload type")
fiber.StatusBadRequest

- Create private function defaultErrorHandler
- Create `PayloadCreator` function type
- Add docs to some function types
- Add doccomment to NewPayload
- Create
    - Test_PASETO_MissingToken
    - Test_PASETO_ErrDataUnmarshal
    - Test_PASETO_ErrTokenExpired
    - Test_Config_Invalid_SymmetricKey
@Jictyvoo
Copy link
Contributor Author

@Jictyvoo hmm, you said you are through, however i don't see any new testcases for the errors ?

can you please include a test for these different errors where you check the correct misbehavior

some should already exist, some not yet

ErrExpiredToken  = errors.New("token has expired")
ErrMissingToken  = errors.New("missing PASETO token")
ErrDataUnmarshal = errors.New("can't unmarshal token data to Payload type")
fiber.StatusBadRequest

Added some tests to cover that. But to do that I detach the defaultErrorHandler to be a const function instead of a anonymous function

Also, what you think about the change of Payload struct to paseto.JSONToken?

Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

Thx

paseto/config.go Outdated Show resolved Hide resolved
@ReneWerner87 ReneWerner87 merged commit cff58a2 into gofiber:main May 28, 2021
@ReneWerner87
Copy link
Member

sorry forgot to answer your question

Also, what you think about the change of Payload struct to paseto.JSONToken?

is currently okay for me, if we get problems with it, we can still change it, software grows as you use it

@Jictyvoo
Copy link
Contributor Author

sorry forgot to answer your question

Also, what you think about the change of Payload struct to paseto.JSONToken?

is currently okay for me, if we get problems with it, we can still change it, software grows as you use it

No problem. Now I gonna think if it's good to add a Storage parameter to the middleware to stores token/uuid to add a verification to it

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

Successfully merging this pull request may close these issues.

move "bad: missing PASETO token" to helpers.go
2 participants