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

Revocable sessions #3616

Merged
merged 3 commits into from
Jun 23, 2017
Merged

Revocable sessions #3616

merged 3 commits into from
Jun 23, 2017

Conversation

sorin-davidoi
Copy link
Contributor

@sorin-davidoi sorin-davidoi commented Jun 6, 2017

As discussed in #3243, this introduces revocable sessions as explained here.

Left to do:

  • Expose the maximum number of sessions as a configuration option (in config/environments?)
  • Make tests pass (see this discussion)

@akihikodaki akihikodaki added security Security issues and fixes, vulnerabilities work in progress Not to be merged, currently being worked on enhancement labels Jun 8, 2017
@sorin-davidoi
Copy link
Contributor Author

Where should the maximum number of session be set? Was thinking of config/settings.yml, but it seams that all those options are configurable from the Web UI.

@Gargron
Copy link
Member

Gargron commented Jun 8, 2017

@sorin-davidoi Probably a config/initializers/ file. What maximum number of sessions do you mean though? Why should there be a maximum?

@sorin-davidoi
Copy link
Contributor Author

With this modification, each login creates a new entry in the session_activations table. If the user frequently switches devices (or uses "private browsing" or just logs in frequently), I imagine this could lead to some space problems. So we place a limit on the number of active sessions (if LIMIT is 5 and your current login creates the sixth session, the first one gets deleted).

@Gargron
Copy link
Member

Gargron commented Jun 8, 2017

@sorin-davidoi I wonder if it would be more appropriate to store sessions in Redis rather than Postgres.

@sorin-davidoi
Copy link
Contributor Author

Not familiar at all with Redis, but I guess that would make #3243 harder? We need to link the subscription to the session that created it, to make sure that they get deleted together.

@sorin-davidoi
Copy link
Contributor Author

sorin-davidoi commented Jun 11, 2017

Added the configuration option (defaulting to a maximum of 10 active sessions per user).

@sorin-davidoi
Copy link
Contributor Author

Anything else I can do here to help with this?

@Gargron
Copy link
Member

Gargron commented Jun 17, 2017

It'd be cool to have some input from @mjankowski or @krainboltgreene on this, looks okay to me but perhaps they had to implement something similar for different applications.

@sorin-davidoi sorin-davidoi mentioned this pull request Jun 22, 2017
23 tasks
@sorin-davidoi
Copy link
Contributor Author

sorin-davidoi commented Jun 23, 2017

Rebased, moved the migration date to today (I guess this is the right thing, correct?).

@Gargron Gargron merged commit 2211e8d into mastodon:master Jun 23, 2017
@sorin-davidoi sorin-davidoi deleted the device-revocable-sessions branch June 23, 2017 16:51
This was referenced Jun 23, 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 work in progress Not to be merged, currently being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants