Skip to content

Commit

Permalink
Improve SSO security
Browse files Browse the repository at this point in the history
Signed-off-by: Walker Crouse <walkercrouse@hotmail.com>
  • Loading branch information
windy1 committed Jan 5, 2017
1 parent e0feb6a commit 1837b0c
Show file tree
Hide file tree
Showing 11 changed files with 99 additions and 14 deletions.
27 changes: 24 additions & 3 deletions app/controllers/Actions.scala
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package controllers

import java.util.Date

import controllers.Requests._
import db.impl.OrePostgresDriver.api._
import db.access.ModelAccess
import db.impl.access.{OrganizationBase, ProjectBase, UserBase}
import models.project.Project
import models.user.User
import models.user.{SignOn, User}
import ore.permission.scope.GlobalScope
import ore.permission.{EditSettings, HideProjects, Permission}
import org.spongepowered.play.security.SingleSignOnConsumer
Expand All @@ -23,6 +27,7 @@ trait Actions extends Calls with ActionHelpers {
val projects: ProjectBase
val organizations: OrganizationBase
val sso: SingleSignOnConsumer
val signOns: ModelAccess[SignOn]

val PermsLogger = play.api.Logger("Permissions")

Expand Down Expand Up @@ -180,6 +185,22 @@ trait Actions extends Calls with ActionHelpers {

}

/**
* Returns true and marks the nonce as used if the specified nonce has not
* been used, has not exired.
*
* @param nonce Nonce to check
* @return True if valid
*/
def isNonceValid(nonce: String): Boolean = this.signOns.find(_.nonce === nonce).exists { signOn =>
if (signOn.isCompleted || new Date().getTime - signOn.createdAt.get.getTime > 600000)
false
else {
signOn.setCompleted()
true
}
}

// Implementation

private def userLock(redirect: Call) = new ActionFilter[AuthRequest] {
Expand All @@ -196,11 +217,11 @@ trait Actions extends Calls with ActionHelpers {
if (sso.isEmpty || sig.isEmpty)
Some(Unauthorized)
else {
Actions.this.sso.authenticate(sso.get, sig.get) match {
Actions.this.sso.authenticate(sso.get, sig.get)(isNonceValid) match {
case None =>
Some(Unauthorized)
case Some(spongeUser) =>
if (spongeUser.username.equals(request.user.username))
if (spongeUser.id == request.user.id.get)
None
else
Some(Unauthorized)
Expand Down
4 changes: 4 additions & 0 deletions app/controllers/BaseController.scala
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package controllers

import db.ModelService
import db.access.ModelAccess
import db.impl.VersionTable
import db.impl.access.{OrganizationBase, ProjectBase, UserBase}
import models.project.{Project, Version}
import models.user.SignOn
import ore.{OreConfig, OreEnv}
import org.spongepowered.play.security.SingleSignOnConsumer
import play.api.i18n.I18nSupport
Expand All @@ -25,6 +27,8 @@ abstract class BaseController(implicit val env: OreEnv,
implicit override val projects: ProjectBase = this.service.getModelBase(classOf[ProjectBase])
implicit override val organizations: OrganizationBase = this.service.getModelBase(classOf[OrganizationBase])

override val signOns: ModelAccess[SignOn] = this.service.access[SignOn](classOf[SignOn])

/**
* Executes the given function with the specified result or returns a
* NotFound if not found.
Expand Down
18 changes: 12 additions & 6 deletions app/controllers/Users.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package controllers

import java.util.Date
import javax.inject.Inject

import db.ModelService
Expand All @@ -8,7 +9,7 @@ import db.impl.access.UserBase.ORDERING_PROJECTS
import discourse.OreDiscourseApi
import form.OreForms
import models.user.role.RoleModel
import models.user.{Notification, User}
import models.user.{Notification, SignOn, User}
import ore.rest.OreWrites
import ore.user.notification.InviteFilters.InviteFilter
import ore.user.notification.NotificationFilters.NotificationFilter
Expand Down Expand Up @@ -45,7 +46,9 @@ class Users @Inject()(fakeUser: FakeUser,
* @return Logged in page
*/
def signUp() = Action { implicit request =>
redirectToSso(this.sso.getSignupUrl(this.baseUrl + "/login"))
val nonce = SingleSignOnConsumer.nonce
this.signOns.add(SignOn(nonce = nonce))
redirectToSso(this.sso.getSignupUrl(this.baseUrl + "/login", nonce))
}

/**
Expand All @@ -62,11 +65,12 @@ class Users @Inject()(fakeUser: FakeUser,
this.users.getOrCreate(this.fakeUser)
this.redirectBack(returnPath.getOrElse(request.path), this.fakeUser)
} else if (sso.isEmpty || sig.isEmpty) {
// Check if forums are available and redirect to login if so
redirectToSso(this.sso.getLoginUrl(this.baseUrl + "/login"))
val nonce = SingleSignOnConsumer.nonce
this.signOns.add(SignOn(nonce = nonce))
redirectToSso(this.sso.getLoginUrl(this.baseUrl + "/login", nonce))
} else {
// Redirected from SpongeSSO, decode SSO payload and convert to Ore user
this.sso.authenticate(sso.get, sig.get) match {
this.sso.authenticate(sso.get, sig.get)(isNonceValid) match {
case None =>
Redirect(ShowHome).withError("error.loginFailed")
case Some(spongeUser) =>
Expand All @@ -85,7 +89,9 @@ class Users @Inject()(fakeUser: FakeUser,
* @return Redirect to verification
*/
def verify(returnPath: Option[String]) = Authenticated { implicit request =>
redirectToSso(this.sso.getVerifyUrl(this.baseUrl + returnPath.getOrElse("/")))
val nonce = SingleSignOnConsumer.nonce
this.signOns.add(SignOn(nonce = nonce))
redirectToSso(this.sso.getVerifyUrl(this.baseUrl + returnPath.getOrElse("/"), nonce))
}

private def redirectToSso(url: String): Result = {
Expand Down
11 changes: 10 additions & 1 deletion app/db/impl/schema.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import db.table.{AssociativeTable, ModelTable, NameColumn}
import models.project._
import models.statistic.{ProjectView, VersionDownload}
import models.user.role.{OrganizationRole, ProjectRole, RoleModel}
import models.user.{Notification, Organization, User, Session => DbSession}
import models.user.{Notification, Organization, SignOn, User, Session => DbSession}
import ore.Colors.Color
import ore.permission.role.RoleTypes.RoleType
import ore.project.Categories.Category
Expand Down Expand Up @@ -175,6 +175,15 @@ class SessionTable(tag: Tag) extends ModelTable[DbSession](tag, "user_sessions")

}

class SignOnTable(tag: Tag) extends ModelTable[SignOn](tag, "sign_ons") {

def nonce = column[String]("nonce")
def isCompleted = column[Boolean]("is_completed")

def * = (id.?, createdAt.?, nonce, isCompleted) <> (SignOn.tupled, SignOn.unapply)

}

class OrganizationTable(tag: Tag) extends ModelTable[Organization](tag, "organizations") with NameColumn[Organization] {

override def id = column[Int]("id", O.PrimaryKey)
Expand Down
4 changes: 3 additions & 1 deletion app/db/impl/service/OreModelConfig.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import db.{ModelSchema, ModelService}
import models.project._
import models.statistic.{ProjectView, VersionDownload}
import models.user.role.{OrganizationRole, ProjectRole}
import models.user.{Notification, Organization, User}
import models.user.{Notification, Organization, SignOn, User}

trait OreModelConfig extends ModelService with OreDBOs {

Expand Down Expand Up @@ -56,6 +56,8 @@ trait OreModelConfig extends ModelService with OreDBOs {

val SessionSchema = new ModelSchema[models.user.Session](this, classOf[models.user.Session], TableQuery[SessionTable])

val SignOnSchema = new ModelSchema[SignOn](this, classOf[SignOn], TableQuery[SignOnTable])

val ProjectRolesSchema = new ModelSchema[ProjectRole](this, classOf[ProjectRole], TableQuery[ProjectRoleTable])

val ProjectSchema = new ProjectSchema(this, Users)
Expand Down
1 change: 1 addition & 0 deletions app/db/impl/service/OreModelService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class OreModelService @Inject()(override val env: OreEnv,
// Register model schemas
registerSchema(UserSchema)
registerSchema(SessionSchema)
registerSchema(SignOnSchema)
registerSchema(ProjectRolesSchema)
registerSchema(ProjectSchema)
registerSchema(ProjectSettingsSchema)
Expand Down
5 changes: 4 additions & 1 deletion app/db/impl/table/ModelKeys.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import db.table.key._
import models.project._
import models.statistic.StatEntry
import models.user.role.RoleModel
import models.user.{Notification, User}
import models.user.{Notification, SignOn, User}
import ore.Colors.Color
import ore.permission.role.RoleTypes.RoleType
import ore.project.Categories.Category
Expand Down Expand Up @@ -58,6 +58,9 @@ object ModelKeys {
val AvatarUrl = new StringKey[User](_.avatarUrl, _.avatarUrl.orNull)
val ReadPrompts = new Key[User, List[Prompt]](_.readPrompts, _.readPrompts.toList)

// SignOn
val IsCompleted = new BooleanKey[SignOn](_.isCompleted, _.isCompleted)

// Version
val IsReviewed = new BooleanKey[Version](_.isReviewed, _.isReviewed)
val ChannelId = new IntKey[Version](_.channelId, _.channelId)
Expand Down
26 changes: 26 additions & 0 deletions app/models/user/SignOn.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package models.user

import java.sql.Timestamp

import db.impl.SignOnTable
import db.impl.model.OreModel
import db.impl.table.ModelKeys._

case class SignOn(override val id: Option[Int] = None,
override val createdAt: Option[Timestamp] = None,
nonce: String,
var _isCompleted: Boolean = false) extends OreModel(id, createdAt) {

override type M = SignOn
override type T = SignOnTable

def isCompleted: Boolean = this._isCompleted

def setCompleted(completed: Boolean = true) = Defined {
this._isCompleted = completed
update(IsCompleted)
}

override def copyWith(id: Option[Int], theTime: Option[Timestamp]) = this.copy(id = id, createdAt = theTime)

}
4 changes: 2 additions & 2 deletions build.sbt
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name := "ore"

version := "1.1.9"
version := "1.1.10"

lazy val `ore` = (project in file(".")).enablePlugins(PlayScala)

Expand All @@ -20,7 +20,7 @@ resolvers ++= Seq(
)

libraryDependencies ++= Seq(
"org.spongepowered" % "sponge-play" % "1.0.0-SNAPSHOT",
"org.spongepowered" % "sponge-play" % "1.0.1-SNAPSHOT",
"org.spongepowered" % "play-discourse" % "1.0.0-SNAPSHOT",
"org.spongepowered" % "plugin-meta" % "0.2",
"com.typesafe.play" %% "play-slick" % "2.0.0",
Expand Down
12 changes: 12 additions & 0 deletions conf/evolutions/default/63.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# --- !Ups

create table sign_ons(
id bigserial not null primary key,
created_at timestamp not null,
nonce varchar(255) not null unique,
is_completed boolean not null default false
);

# --- !Downs

drop table sign_ons;
1 change: 1 addition & 0 deletions conf/logback.xml
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,6 @@
<logger name="Permissions" level="INFO" />
<logger name="UserSync" level="INFO" />
<logger name="Organizations" level="INFO" />
<logger name="SSO" level="INFO" />

</configuration>

0 comments on commit 1837b0c

Please sign in to comment.