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

Accesstoken generation fails, user_agent field has no default #136

Closed
mrvdb opened this issue Jul 9, 2019 · 11 comments

Comments

@mrvdb
Copy link
Collaborator

commented Jul 9, 2019

The field accesstokens.user_agent is set to not null but has no default and the code does not explicitly set it on inserting an accesstoken. When trying to generate an accesstoken through the API, it fails because of this.

Reproduce:

curl "https://site.name/api/auth/login" \
  -H "Content-Type: application/json" -X POST \     
  -d '{"alias": "[user]", "pass": "[pass]"}

This will generate an error for the user. Writefreely log shows:

ERROR: 2019/07/09 08:59:48 log.go:26: Login: Unable to create access token: Error 1364: Field 'user_agent' doesn't have a default value

Workaround:
alter table accesstokens change user_agent user_agent varchar(255) not null default "";

If it is indeed intended that user_agent should not be null/empty then the code needs to reflect this.

@thebaer thebaer added the bug label Jul 9, 2019

@thebaer

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Thanks for the report, @mrvdb. I think this would be a good opportunity to start using that column for what it's originally there for -- storing the User-Agent header that comes with the request that first generated the token.

@mrvdb

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 31, 2019

Agreed. The proposed change of the column definition should still be part of that resolution as well I think.

@robjloranger

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

I can't reproduce this. Is this a user who has never logged in without sending the user agent?

@thebaer

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

The error will only manifest on newer versions of MySQL, I believe.

@robjloranger

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

oh ok, so not with sqlite3?

@robjloranger

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Also the schema appears to be setting accesstokens.user_agent to DEFAULT NULL. Is that not explicitly allow null by default?

Still learning some SQL things so maybe I'm misinterpreting?

@mrvdb

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 13, 2019

Also the schema appears to be setting accesstokens.user_agent to DEFAULT NULL. Is that not explicitly allow null by default?

That's odd, the schema our mysql/mariadb was using did definitely not have the DEFAULT NULL definition, but a NOT NULL clause. Is schema.sql the actual source for creating/updating the schema?

I see there is a byte representation of the schema in bindata-lib.go How is this related to the schema.sql? Are they kept in sync?

@mrvdb

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 13, 2019

It seems to be a migration issue, see a3e287a

Our db was created before that revision and migration never picked up that change. Not sure how to get to T529 which is referred to in that revision.

@robjloranger

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

That would be over on phabricator, https://phabricator.write.as/T529

@mrvdb

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 13, 2019

Thanks, so local fixing resolves this particular issue. That it slipped by migration is another issue ;-)

I sort of assumed the migration would be based on some hash-function over the schema.sql input (per db presumably), thus guaranteeing to at least syntactically catching all changes to the schema.

@mrvdb mrvdb closed this Aug 13, 2019

@robjloranger

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Awesome, glad we got you sorted.

I'm not sure how we're doing it now, but something like that would be ideal. Guaranteed migrations or explicit failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.