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.

Sign up for GitHub

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

Jump to bottom

if auth_opt_jwt_skip_user_expiration enabled, in case of receive bad token the code crashes. also README descriptions added. #337

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

if auth_opt_jwt_skip_user_expiration enabled, in case of receive bad token the code crashes. also README descriptions added. #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b602a6
Update README.md
mahdizadsar Sep 20, 2024
245a01a
Update jwt.go
mahdizadsar Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1000,11 +1000,14 @@ auth_opt_jwt_userquery select count(*) from test_user where username = $1 limit
Thus, the following specific JWT local options are supported:


| 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 |

| 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` |
Comment on lines +1003 to +1010
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

@iegomez iegomez Sep 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


Notice that general `jwt_secret` is mandatory when using this mode.
`jwt_userfield` is still optional and serves as a mean to extract the username from either the claim's `Subject` (`sub` field),
Expand All @@ -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
Copy link
Owner

Choose a reason for hiding this comment

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

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 ?.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I'm not against the addition but the replacement.

```

*Important note:*
Expand Down
20 changes: 12 additions & 8 deletions backends/jwt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,19 @@ func getJWTClaims(secret string, tokenStr string, skipExpiration bool) (*jwtGo.M
})

expirationError := false
if err != nil {
if !skipExpiration {
log.Debugf("jwt parse error: %s", err)
return nil, err
}

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
}
Comment on lines +135 to +147
Copy link
Owner

Choose a reason for hiding this comment

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

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
		}
	}

Copy link
Owner

Choose a reason for hiding this comment

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

On another note, I'd also add a test with some wrong token that previously would crash and now doesn't.

Copy link
Author

Choose a reason for hiding this comment

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

great good job

Copy link
Owner

Choose a reason for hiding this comment

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

@mahdizadsar let me know if you plan on adding that test and addressing other concerns so we may merge.

}

if !jwtToken.Valid && !expirationError {
Expand Down
Loading