Skip to content

feat: add configuration to set an URL parameter#51

Open
nicolas-brousse wants to merge 1 commit intolinqueta:masterfrom
nicolas-brousse:add-token-params-configuration
Open

feat: add configuration to set an URL parameter#51
nicolas-brousse wants to merge 1 commit intolinqueta:masterfrom
nicolas-brousse:add-token-params-configuration

Conversation

@nicolas-brousse
Copy link

This PR add a token configuration to add a small protection on URL.

@linqueta
Copy link
Owner

Hey @nicolas-brousse , I think this part of controller is growing with some features that was implemented recently. So, at today night I'll refactor and include your suggestion.

Can be by this way?

@nicolas-brousse
Copy link
Author

@linqueta sure 🙂

Copy link
Owner

@linqueta linqueta left a comment

Choose a reason for hiding this comment

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

I think we can merge it now, so, can you adjust the token condition?

@nicolas-brousse
Copy link
Author

Hi @linqueta. What is the issue with the token condition? I'm not sure to understand what you want as changes.

Comment on lines +35 to +37
return true if Healthcheck.configuration.token.blank?

Healthcheck.configuration.token == params[:token]
Copy link
Owner

@linqueta linqueta May 14, 2021

Choose a reason for hiding this comment

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

Sorry man, I forgot to send this comment.

I just asked about to replace it to:

Suggested change
return true if Healthcheck.configuration.token.blank?
Healthcheck.configuration.token == params[:token]
Healthcheck.configuration.token.blank? || Healthcheck.configuration.token == params[:token]

@linqueta
Copy link
Owner

Hi @linqueta. What is the issue with the token condition? I'm not sure to understand what you want as changes.

Sorry about my mistake, I forgot to send the comment with the adjust, can you see it now?

@nicolas-brousse
Copy link
Author

@linqueta yes I see it. I'll try to have a look on it this week. Thanks

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.

2 participants