if auth_opt_jwt_skip_user_expiration enabled, in case of receive bad token the code crashes. also README descriptions added.
#337
Conversation
ACL in MySQL local mode configuration had some problems, added some explanation for using ACL using MySQL database in local mode.
issue: When `auth_opt_jwt_skip_user_expiration` is enabled in config file and the wrong JWT token is sent by client to server (with a few or completely wrong segments), the code crashes. Workaround: modify the code structure by moving the checking of token expiration conditions
| | Option | default | Mandatory | Meaning | | ||
| | ----------------------------- | --------- | :-------: | -------------------------------------------------------- | | ||
| | auth_opt_jwt_db | postgres | N | The DB backend to be used, either `postgres` or `mysql` | | ||
| | auth_opt_jwt_userquery | | Y | SQL query for users | | ||
| | auth_opt_jwt_mysql_dbname | | Y/N | must set if auth_opt_jwt_db set is `mysql` | | ||
| | auth_opt_jwt_mysql_user | | Y/N | must set if auth_opt_jwt_db set is `mysql` | | ||
| | auth_opt_jwt_mysql_password | | Y/N | must set if auth_opt_jwt_db set is `mysql` | | ||
| | auth_opt_jwt_mysql_aclquery | | Y/N | ACL query must set if auth_opt_jwt_db set is `mysql` | |
There was a problem hiding this comment.
I agree the explanation could be a bit better, but it's all stated in https://github.com/iegomez/mosquitto-go-auth?tab=readme-ov-file#local-mode.
My nitpick here is that this table doesn't include all the rest of the options that are still valid, albeit not mandatory, but doing so for both PG and MySQL is a bit repetitive.
So maybe just change a bit the wording instead?
There was a problem hiding this comment.
I agree, but I suggest that the tables should be completed in order to explicitly specify which items are mandatory for a particular situation. In this case, to use MySQL in JWT mode, the fields I wrote seem mandatory
There was a problem hiding this comment.
Sure, I was just saying that besides the user query, which becomes irrelevant, all the options still follow the same mandatoriness that's spelled on PG and MySQL sections.
That said, I'm not really against being super clear, so if you want to throw in exhaustive tables for both DBs in the JWT case, all the better.
| @@ -1022,7 +1025,7 @@ auth_opt_jwt_userquery select count(*) from "user" where username = $1 and is_ac | |||
| For mysql: | |||
|
|
|||
| ``` | |||
| auth_opt_jwt_userquery select count(*) from "user" where username = ? and is_active = true limit 1 | |||
| auth_opt_jwt_mysql_aclquery select count(*) from "user" where username = ? and is_active = true limit 1 | |||
There was a problem hiding this comment.
There's no need to remove the valid auth_opt_jwt_userquery.
In fact, this is showing the difference between PG and MySQL params, i.e. $1 versus ?.
There was a problem hiding this comment.
right, but you wrote an example for PG in a few lines earlier. This line is an example for MySQL, however, this is similar to the previous topic.
There was a problem hiding this comment.
Yeah, I'm not against the addition but the replacement.
| if err != nil { | ||
| if v, ok := err.(*jwtGo.ValidationError); ok && v.Errors == jwtGo.ValidationErrorExpired { | ||
| expirationError = true | ||
| } | ||
| log.Debugf("token expired: %s", err) | ||
| if skipExpiration { | ||
| expirationError = true | ||
| }else{ | ||
| log.Debugf("jwt parse error: %s", err) | ||
| return nil, err | ||
| } | ||
| }else{ | ||
| log.Debugf("jwt parse error: %s", err) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
The format is a bit weird, Go's formatter should have caught missing spaces.
Also, I'd structure it like this instead to skip the inner else clause by returning early on !skipExpiration:
if err != nil {
if v, ok := err.(*jwtGo.ValidationError); ok && v.Errors == jwtGo.ValidationErrorExpired {
log.Debugf("jwt token expired: %s", err)
if !skipExpiration {
return nil, err
}
expirationError = true
} else {
log.Debugf("jwt parse error: %s", err)
return nil, err
}
}
There was a problem hiding this comment.
On another note, I'd also add a test with some wrong token that previously would crash and now doesn't.
There was a problem hiding this comment.
@mahdizadsar let me know if you plan on adding that test and addressing other concerns so we may merge.
issue 1: In README.md ACL in MySQL local mode configuration had some problems,
Workaround: I realized that it should be explained to the user what parts are required to use the mysql database in JWT mode. Also to use ACL In JWT mode,
auth_opt_jwt_mysql_aclquerymust be set with the correct query. this fields are:auth_opt_jwt_mysql_dbname,auth_opt_jwt_mysql_user,auth_opt_jwt_mysql_password,auth_opt_jwt_mysql_aclqueryissue 2: When
auth_opt_jwt_skip_user_expirationis enabled in config file and the wrong JWT token is sent by client to server (with a few or completely wrong segments), the code crashes.Workaround: modify the code structure by moving the checking of token expiration conditions.