Fixes #36972 - Make hammer accept unlimited as JWT life time

[girijaasoni]

No description provided.

[theforeman-bot]

Can one of the admins verify this patch?

[theforeman-bot]

Can one of the admins verify this patch?

[theforeman-bot]

Can one of the admins verify this patch?

[sayan3296]

As a person of interest, I wanted to ask something. Right now the jwt_expiration flag accepts both integers as well as the unlimited string but that raises some concern as explained in theforeman/foreman-ansible-modules#1683 (comment) .

Is there an alternative way to generate the token for unlimited time using some numeric\interger inputs instead of using unlimited string ?

[stejskalleos]

As @sayan3296 mentioned, going with :number only would be probably better and safer way to go.
Plus we could (should?) unify the behavior with UI controller as well.

[stejskalleos]

This should allow both: number and string as well. But I think we should go only with :number only, with following logic:

• x < 0 raise an error, invalid value
• x = 0 unlimited expiration
• x > 0 expiration in hours.

This should be also described in the documentation

[stejskalleos]

I forgot about the default value, if not set , default value should be 4 (same as in the UI)

[girijaasoni]

That makes sense @stejskalleos , thank you

[ofedoren]

What about leaving negative numbers as valid, but treat them as unlimited expiration? I mean, 0 can be used for test purposes to simulate expired tokens without a need to actually wait.

[sayan3296]

In that case, perhaps
x < -1 raises an error, invalid value
x = 0 expired
x = -1 Unlimited token
x > 0 expiration in hours.

[girijaasoni]

As discussed with @ares on our 1:1, it's a good idea to verify with the QEs if they actually use 0 for testing purposes (from UI) , can double check for api as well, accordingly we can either use '-1' or '0' and incline UI in the similar manner. I will also verify the convention with the reporter and update the PR.

[pablomh]

I get the following when testing this patch:

ERROR: unauthorized
JWT SSO: Expired JWT token.

[girijaasoni]

I get the following when testing this patch:

ERROR: unauthorized
JWT SSO: Expired JWT token.

That's weird. It could be because the API is modified to accept strings as well.

[stejskalleos]

What's the use-case for it? Why would the user want to create a registration command with an expired token?

[girijaasoni]

I think so too, I spoke with QAs, they do not use it for testing either. I think it would be safe to use '0' to represent 'unlimited'
@pablomh what is your opinion on using 0 for unlimited instead of -1?

[pablomh]

I agree with @stejskalleos and I think that we should avoid accepting an "expired token option" and settle on:

• x < 0 raises an error, invalid value
• x = 0 Unlimited token
• x > 0 expiration in hours

@sayan3296, opinions?

[sayan3296]

I apologize for the late reply and that sounds fine to me as long as everyone else agrees.

[stejskalleos]

This is not readable much, maybe the case statement here would be better.
Also why to have the condition for the unlimited, that should be changed to -1 right?

[girijaasoni]

The value of jwt_args[:expiration] must be nil when the token value is 'unlimited' (from the UI) or -1 from the api or hammer so this line is basically not assigning anything to jwt_args[:expiration] when the either of them is the case.
We can also convert 'unlimited' (coming from the UI) to -1 and then send nil to jwt_args[:expiration] when the case is -1 but that's just an extra step. Whatever works.
I agree with the case statements. I can try that

[stejskalleos]

Should we explicitly mention all the possibilities? -2..,-1,0, 1, max value(999999)

[girijaasoni]

To align it with UI, its a good idea to set the max value to 999999.

[pablomh]

Current patch works in my testing environment.

[evgeni]

Some nitpicks:

• The API has no option --jwt-expiration, that's a hammer thing. I think here we should talk about the jwt_expiration parameter.
• I think it's more correct to raise a Foreman::Exception like this: raise ::Foreman::Exception.new(N_("the text")) (see https://projects.theforeman.org/projects/foreman/wiki/Translating#Exceptions)
• Why 999999? This seems like an arbitrary number without any meaning? It translates to roughly 114 years which seems rather long. We either should have a "sensible" limit, like a year or two. Or no limit at all (my preference).

[girijaasoni]

Thank you so much @evgeni , I'll update the exception, Foreman exception is definitely more valid.
I used 999999 because that's the limit set from the UI validations I think when are conventionally accepting unlimited, then 999999 hours(which is still limited) might make sense. I could remove the limit all together but that would not incline with the UI.

[ShimShtein]

Can we extract the registration_params['jwt_expiration'] into a method, to make the jwt_expiration constant appear once?
Something like this:

def jwt_expiration_param
  # use require if the parameter is mandatory or permit if it's optional.
  param = registration_params.require('jwt_expiration') || 4
  @jwt_expiration_param ||= begin
    if param == 'unlimited'
      0
    else
      Integer(param)
    end
end

Then the expiration_unlimited would not have to handle with string values at all:

def def expiration_unlimited?
  jwt_expiration_param == 0
end

[girijaasoni]

Thanks Shim. This was more readable.
A few points:

• permit method returns the value as a Parameter object. So I'm unable to use to_i or Integer on it
• I have used param.to_i instead of Integer(param) because it would break the code if we input a decimal value from the UI (which currently is converted to greatest integer with the to_i method). Also integer validation is automatically happening for hammer and api here with the number validator

[evgeni]

Sadly, we do not enable Apipie validations in Foreman (for 11 years now: 5d78633 -- this is why the UI can pass unlimited, even if the api says it's a number):

foreman/config/initializers/apipie.rb

Lines 13 to 14 in a1142ab

	# TODO enable?
	config.validate = false

Some clients (e.g. FAM) still use the values in the APIDoc to enforce validation, but that's up to the clients.

[ShimShtein]

This implementation will not fail validation of strings other than unlimited.

Since you have to accept decimal values, you can still have it validated:

Suggested change

	param.to_i
	Float(param).to_i

This will accept 11.5, but will fail for unknown words.

[stejskalleos]

Two nits

[stejskalleos]

If we have constants for MIN_VALUE and MAX_VALUE, shouldn't we have also a constant for the default value?

[girijaasoni]

MIN_VALUE and MAX_VALUE are used twice (in expiration_valid? and in the error message)
We're using default value only once

[stejskalleos]

We should catch an error when user sends invalid data directly to the API:

curl -X POST "http://localhost:3000/api/registration_commands" \
     -H "Content-Type: application/json" \
     --user admin:changeme \
     -d '{"registration_command": {"jwt_expiration": "unlimiteded"}}'

=>

{
  "error": {"message":"invalid value for Float(): \"unlimiteded\""}
}

[girijaasoni]

Could you elaborate more? I tested this and as far as I know , we are already catching the error. Am I missing something?

[stejskalleos]

Well yes, but the error is kinda programmatic, not user-friendly.
Instead of invalid value for Float(): \"unlimiteded\" we can raise Invalid value %s for jwt_expiration. The value must be between %s and %s. 0 means 'unlimited'.

[girijaasoni]

validation is done in the following order :

1. Float() is to invalidate Strings except 'unlimited'
2. Invalid value %s for jwt_expiration. The value must be between 0 and 999999. 0 means 'unlimited'. is basically to invalidate negative numbers

I think it makes sense to have 2 different errors for 2 different types of validations. I could however try to update it but that could mean changing the order(which is a lot of modification) or I could pass -1 as a value for strings, if it's absolutely required

[stejskalleos]

We probably talk about two different things, but my main concern is the message itself.
As a user, I don't want to get an invalid value for Float() error, I want to get easy to follow, descriptive error message, like The value must be between 0 and 999999. 0 means 'unlimited'.

[sayan3296]

+1

A single and easily understandable error message is always preferable when you consider the user-experience for our product.

The value must be between 0 and 999999. Here 0 represents 'unlimited'. --> This sounds great to me.

[girijaasoni]

Thanks @stejskalleos, @sayan3296 , I was unaware that we can set Float exceptions to false. Updated it :D

[stejskalleos]

I like the code, LGTM, just lacks the test coverage. Can you add the following tests:

• With expiration value below 0 (-1)
• With expiration value above max value
• Trying to generate command with nonsense string

With this test coverage, we should be safe.

[girijaasoni]

I like the code, LGTM, just lacks the test coverage. Can you add the following tests:

• With expiration value below 0 (-1)
• With expiration value above max value
• Trying to generate command with nonsense string

With this test coverage, we should be safe.

Nice catch @stejskalleos , updated it

[stejskalleos]

🍏 LGTM

Works as expected, nice test coverage, let's get this in.

Thanks @girijaasoni and everyone involved!

[ekohl]

This broke translations:

MALFORMED: "Invalid value '%s' for jwt_expiration. The value must be between %s and %s. 0 "

I think this is because you can only use %s once. More than that and it becomes ambiguous.

#10176