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

Bind web UI access tokens to sessions #3940

Merged
merged 7 commits into from Jun 25, 2017

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Jun 25, 2017

PR based on #3929

When you logout, session also destroys the access token, so it's no longer
valid. If access token is destroyed some other way, the session is also
destroyed, requiring a re-login.

Fix #1681 - Add scheduler to remove revoked access tokens and grants
@Gargron Gargron added the security Security issues and fixes, vulnerabilities label Jun 25, 2017
Copy link
Contributor

@beatrix-bitrot beatrix-bitrot left a comment

Choose a reason for hiding this comment

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

lgtm~

@Gargron Gargron merged commit ed7dc17 into master Jun 25, 2017
@Gargron Gargron deleted the feature-session-bound-access-tokens branch June 25, 2017 21:51
def assign_access_token
superapp = Doorkeeper::Application.find_by(superapp: true)

return if superapp.nil?
Copy link
Contributor

@unarist unarist Jun 26, 2017

Choose a reason for hiding this comment

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

Since this check hasn't existed previously, it will breaks instances which don't have "Web" application. For example, Docker Guide says you should run docker-compose run --rm web rake db:migrate, but db:migrate doesn't call db:seed which contains "Web" app generation.

Anyway, super app must be existent, so we shouldn't ignore non existence of it I think. If you don't want to regenerate this automatically, you might want to mention this in the release note.

Copy link
Member Author

Choose a reason for hiding this comment

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

This must have existed previously. The old code was in HomeController#find_or_create_access_token, which also called Doorkeeper::Application.where(superapp: true).first - how could those instances have worked then? Unless nil was allowed for application_id on oauth_access_tokens, which I didn't check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like our db:migrate does also db:setup now, but it was introduced at v1.4rc4...
256e3ad#diff-1bf337030f4e56fa36281d23cf9e2082

@unarist
Copy link
Contributor

unarist commented Jun 27, 2017

BTW, don't you force users logged out when access_token is missing?

zunda added a commit to zunda/mastodon that referenced this pull request Jul 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Security issues and fixes, vulnerabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants