From ba673943d8e5a8cc96bc80ff9a27cac97b732662 Mon Sep 17 00:00:00 2001 From: Fabrice Lecomte Date: Mon, 18 Jan 2021 09:51:48 +0100 Subject: [PATCH] Refactoring of OpinionVoter --- src/main/kotlin/application/Application.kt | 4 +- src/main/kotlin/application/KoinModule.kt | 2 + .../component/comment/generic/Comment.kt | 2 +- .../component/comment/generic/CommentVoter.kt | 4 +- src/main/kotlin/entity/Extra.kt | 6 +- src/main/kotlin/entity/Follow.kt | 2 +- src/main/kotlin/entity/Opinion.kt | 11 +- src/main/kotlin/entity/Vote.kt | 2 +- src/main/kotlin/repository/Opinion.kt | 19 ++-- src/main/kotlin/routes/OpinionArticle.kt | 30 +++--- src/main/kotlin/voter/OpinionVoter.kt | 65 +++++------- src/test/kotlin/steps/OpinionSteps.kt | 2 +- .../kotlin/unit/voter/OpinionVoterTest.kt | 100 ++++++------------ src/test/resources/feature/opinion.feature | 1 - 14 files changed, 101 insertions(+), 149 deletions(-) diff --git a/src/main/kotlin/application/Application.kt b/src/main/kotlin/application/Application.kt index cad61eb..52a0e35 100644 --- a/src/main/kotlin/application/Application.kt +++ b/src/main/kotlin/application/Application.kt @@ -42,7 +42,6 @@ import fr.dcproject.event.EventNotification import fr.dcproject.event.EventSubscriber import fr.dcproject.routes.* import fr.dcproject.security.voter.OpinionChoiceVoter -import fr.dcproject.security.voter.OpinionVoter import fr.ktorVoter.AuthorizationVoter import fr.ktorVoter.VoterException import fr.postgresjson.migration.Migrations @@ -92,7 +91,6 @@ fun Application.module(env: Env = PROD) { install(AuthorizationVoter) { voters = listOf( - OpinionVoter(), OpinionChoiceVoter() ) } @@ -177,7 +175,7 @@ fun Application.module(env: Env = PROD) { commentConstitution(get(), get()) voteArticle(get(), get(), get(), get()) voteConstitution(get(), get()) - opinionArticle(get()) + opinionArticle(get(), get()) opinionChoice(get()) definition() } diff --git a/src/main/kotlin/application/KoinModule.kt b/src/main/kotlin/application/KoinModule.kt index d9cb0b1..705cbb4 100644 --- a/src/main/kotlin/application/KoinModule.kt +++ b/src/main/kotlin/application/KoinModule.kt @@ -25,6 +25,7 @@ import fr.dcproject.messages.NotificationEmailSender import fr.dcproject.repository.CommentConstitutionRepository import fr.dcproject.security.voter.ConstitutionVoter import fr.dcproject.security.voter.FollowVoter +import fr.dcproject.security.voter.OpinionVoter import fr.dcproject.security.voter.VoteVoter import fr.postgresjson.connexion.Connection import fr.postgresjson.connexion.Requester @@ -129,6 +130,7 @@ val KoinModule = module { single { ConstitutionVoter() } single { VoteVoter() } single { FollowVoter() } + single { OpinionVoter() } // Elasticsearch Client single { diff --git a/src/main/kotlin/component/comment/generic/Comment.kt b/src/main/kotlin/component/comment/generic/Comment.kt index 7830a48..d04403b 100644 --- a/src/main/kotlin/component/comment/generic/Comment.kt +++ b/src/main/kotlin/component/comment/generic/Comment.kt @@ -72,7 +72,7 @@ open class CommentParent( interface CommentParentI : CommentI, EntityDeletedAt, CommentWithTargetI -interface CommentWithTargetI : CommentI, TargetI, AsTarget +interface CommentWithTargetI : CommentI, TargetI, HasTarget interface CommentWithParentI { val parent: CommentParent? diff --git a/src/main/kotlin/component/comment/generic/CommentVoter.kt b/src/main/kotlin/component/comment/generic/CommentVoter.kt index 8008c3a..ca82c52 100644 --- a/src/main/kotlin/component/comment/generic/CommentVoter.kt +++ b/src/main/kotlin/component/comment/generic/CommentVoter.kt @@ -1,7 +1,7 @@ package fr.dcproject.component.comment.generic import fr.dcproject.component.citizen.CitizenI -import fr.dcproject.entity.AsTarget +import fr.dcproject.entity.HasTarget import fr.dcproject.voter.Voter import fr.dcproject.voter.VoterResponse import fr.postgresjson.entity.EntityCreatedBy @@ -23,7 +23,7 @@ class CommentVoter : Voter() { where S : CommentI, S : EntityCreatedBy, S : CommentWithParentI<*>, - S : AsTarget<*> = when { + S : HasTarget<*> = when { citizen == null -> denied("You must be connected to create user", "comment.create.notConnected") subject.createdBy.id != citizen.id -> denied("You cannot create a comment with other user than yours", "comment.create.wrongUser") subject.parent?.isDeleted() ?: false -> denied("You cannot create a comment on deleted parent", "comment.create.deletedParent") diff --git a/src/main/kotlin/entity/Extra.kt b/src/main/kotlin/entity/Extra.kt index dd3f6ea..4261770 100644 --- a/src/main/kotlin/entity/Extra.kt +++ b/src/main/kotlin/entity/Extra.kt @@ -13,11 +13,11 @@ import kotlin.reflect.full.isSubclassOf interface ExtraI : UuidEntityI, - AsTarget, + HasTarget, EntityCreatedAt, EntityCreatedBy -interface AsTarget { +interface HasTarget { val target: T } @@ -45,7 +45,7 @@ interface TargetI : UuidEntityI { t.isSubclassOf(ArticleRef::class) -> TargetName.Article.targetReference t.isSubclassOf(ConstitutionRef::class) -> TargetName.Constitution.targetReference t.isSubclassOf(CommentRef::class) -> TargetName.Comment.targetReference - t.isSubclassOf(Opinion::class) -> TargetName.Opinion.targetReference + t.isSubclassOf(OpinionRef::class) -> TargetName.Opinion.targetReference else -> throw error("target not implemented: ${t.qualifiedName} \nImplement it or return 'reference' from SQL") } } diff --git a/src/main/kotlin/entity/Follow.kt b/src/main/kotlin/entity/Follow.kt index 9789ecb..5129ee3 100644 --- a/src/main/kotlin/entity/Follow.kt +++ b/src/main/kotlin/entity/Follow.kt @@ -29,7 +29,7 @@ class FollowForUpdate( override val target: T, override val createdBy: C ) : FollowRef(id), - AsTarget, + HasTarget, EntityCreatedBy by EntityCreatedByImp(createdBy) open class FollowRef( diff --git a/src/main/kotlin/entity/Opinion.kt b/src/main/kotlin/entity/Opinion.kt index 3386ea0..d05d99e 100644 --- a/src/main/kotlin/entity/Opinion.kt +++ b/src/main/kotlin/entity/Opinion.kt @@ -14,8 +14,8 @@ open class Opinion( override val createdBy: CitizenBasic, override val target: T, val choice: OpinionChoice -) : ExtraI, - TargetRef(id), +) : OpinionRef(id), + ExtraI, EntityCreatedAt by EntityCreatedAtImp(), EntityCreatedBy by EntityCreatedByImp(createdBy) { @@ -32,14 +32,15 @@ class OpinionArticle( data class OpinionForUpdate( override val id: UUID = UUID.randomUUID(), - val target: T, - val choice: OpinionChoice, + override val target: T, + val choice: OpinionChoiceRef, override val createdBy: CitizenRef ) : OpinionRef(id), + HasTarget, EntityCreatedBy by EntityCreatedByImp(createdBy) open class OpinionRef( override val id: UUID -) : OpinionI +) : OpinionI, TargetRef(id) interface OpinionI : UuidEntityI \ No newline at end of file diff --git a/src/main/kotlin/entity/Vote.kt b/src/main/kotlin/entity/Vote.kt index 5c9e810..fcfd3b9 100644 --- a/src/main/kotlin/entity/Vote.kt +++ b/src/main/kotlin/entity/Vote.kt @@ -33,7 +33,7 @@ class VoteForUpdate( VoteForUpdateI, EntityCreatedBy by EntityCreatedByImp(createdBy) -interface VoteForUpdateI : VoteI, AsTarget, EntityCreatedBy { +interface VoteForUpdateI : VoteI, HasTarget, EntityCreatedBy { override val id: UUID val note: Int override val target: T diff --git a/src/main/kotlin/repository/Opinion.kt b/src/main/kotlin/repository/Opinion.kt index a891f0d..5999d1b 100644 --- a/src/main/kotlin/repository/Opinion.kt +++ b/src/main/kotlin/repository/Opinion.kt @@ -2,8 +2,6 @@ package fr.dcproject.repository import com.fasterxml.jackson.core.type.TypeReference import fr.dcproject.component.article.ArticleRef -import fr.dcproject.component.citizen.CitizenRef -import fr.dcproject.entity.OpinionChoiceRef import fr.dcproject.entity.OpinionForUpdate import fr.dcproject.entity.TargetRef import fr.postgresjson.connexion.Paginated @@ -67,9 +65,9 @@ abstract class Opinion(requester: Requester) : OpinionChoice(requ /** * Create an Opinion on target (article,...) */ - abstract fun updateOpinions(choices: List, citizen: CitizenRef, target: TargetRef): List> - fun updateOpinions(choice: OpinionChoiceRef, citizen: CitizenRef, target: TargetRef): List> = - updateOpinions(listOf(choice), citizen, target) + abstract fun updateOpinions(opinions: List>): List> + fun updateOpinions(opinion: OpinionForUpdate<*>): List> = + updateOpinions(listOf(opinion)) abstract fun addOpinion(opinion: OpinionForUpdate): OpinionEntity @@ -135,14 +133,15 @@ class OpinionArticle(requester: Requester) : Opinion(requester) { /** * Update Opinions on Article (Delete old one) */ - override fun updateOpinions(choices: List, citizen: CitizenRef, target: TargetRef): List { + override fun updateOpinions(opinions: List>): List { return requester + /* TODO change SQL function to not use .first() and pass all createdBy and target */ .getFunction("update_citizen_opinions_by_target_id") .select( - "choices_ids" to choices.map { it.id }, - "citizen_id" to citizen.id, - "target_id" to target.id, - "target_reference" to target.reference + "choices_ids" to opinions.map { it.choice.id }, + "citizen_id" to opinions.first().createdBy.id, + "target_id" to opinions.first().target.id, + "target_reference" to opinions.first().target.reference ) } diff --git a/src/main/kotlin/routes/OpinionArticle.kt b/src/main/kotlin/routes/OpinionArticle.kt index e70695a..e398fcf 100644 --- a/src/main/kotlin/routes/OpinionArticle.kt +++ b/src/main/kotlin/routes/OpinionArticle.kt @@ -1,14 +1,14 @@ package fr.dcproject.routes import fr.dcproject.component.article.ArticleForView +import fr.dcproject.component.article.ArticleRef import fr.dcproject.component.auth.citizen +import fr.dcproject.component.auth.citizenOrNull import fr.dcproject.component.citizen.CitizenRef -import fr.dcproject.entity.OpinionChoiceRef -import fr.dcproject.security.voter.OpinionVoter.Action.CREATE -import fr.dcproject.security.voter.OpinionVoter.Action.VIEW +import fr.dcproject.entity.* +import fr.dcproject.security.voter.OpinionVoter import fr.dcproject.utils.toUUID -import fr.ktorVoter.assertCan -import fr.ktorVoter.assertCanAll +import fr.dcproject.voter.assert import io.ktor.application.* import io.ktor.http.* import io.ktor.locations.* @@ -41,7 +41,7 @@ object OpinionArticlePaths { @KtorExperimentalAPI class ArticleOpinion(val article: ArticleForView) { class Body(ids: List) { - val ids = ids.map { OpinionChoiceRef(it.toUUID()) } + val ids: List = ids.map { it.toUUID() } } } @@ -51,29 +51,35 @@ object OpinionArticlePaths { @Location("/citizens/{citizen}/opinions") class CitizenOpinions(val citizen: CitizenEntity, id: List) : KoinComponent { val id: List = id.toUUID() - val opinionsEntities = get() + val opinionsEntities: List> = get() .findCitizenOpinionsByTargets(citizen, this.id) } } @KtorExperimentalAPI @KtorExperimentalLocationsAPI -fun Route.opinionArticle(repo: OpinionArticleRepository) { +fun Route.opinionArticle(repo: OpinionArticleRepository, voter: OpinionVoter) { get { val opinions = repo.findCitizenOpinions(citizen, it.page, it.limit) call.respond(opinions) } get { - assertCanAll(VIEW, it.opinionsEntities) + voter.assert { canView(it.opinionsEntities, citizenOrNull) } call.respond(it.opinionsEntities) } put { - call.receive().ids.let { choices -> - assertCan(CREATE, it.article) - repo.updateOpinions(choices, citizen, it.article) + call.receive().ids.map { id -> + OpinionForUpdate( + choice = OpinionChoiceRef(id), + target = it.article, + createdBy = citizen + ) + }.let { opinions -> + voter.assert { canCreate(opinions, citizenOrNull) } + repo.updateOpinions(opinions) }.let { call.respond(HttpStatusCode.Created, it) } diff --git a/src/main/kotlin/voter/OpinionVoter.kt b/src/main/kotlin/voter/OpinionVoter.kt index 1e6faa6..dd4e5c8 100644 --- a/src/main/kotlin/voter/OpinionVoter.kt +++ b/src/main/kotlin/voter/OpinionVoter.kt @@ -1,48 +1,35 @@ package fr.dcproject.security.voter -import fr.dcproject.component.article.ArticleAuthI -import fr.dcproject.component.article.ArticleForView -import fr.dcproject.component.auth.user -import fr.dcproject.entity.Opinion -import fr.dcproject.voter.NoRuleDefinedException -import fr.dcproject.voter.NoSubjectDefinedException -import fr.ktorVoter.* -import io.ktor.application.* +import fr.dcproject.component.citizen.CitizenI +import fr.dcproject.entity.HasTarget +import fr.dcproject.entity.OpinionI +import fr.dcproject.voter.Voter +import fr.dcproject.voter.VoterResponse +import fr.postgresjson.entity.EntityCreatedBy +import fr.postgresjson.entity.EntityDeletedAt -class OpinionVoter : Voter { - enum class Action : ActionI { - CREATE, - VIEW, - DELETE +class OpinionVoter : Voter() { + + fun canCreate(subjects: List, citizen: CitizenI?): VoterResponse where S : OpinionI, S : HasTarget<*> = + canAll(subjects) { canCreate(it, citizen) } + + fun canCreate(subject: S, citizen: CitizenI?): VoterResponse where S : OpinionI, S : HasTarget<*> { + val target = subject.target + return when { + citizen == null -> denied("You must be connected to make an opinion", "opinion.create.notConnected") + target is EntityDeletedAt && target.isDeleted() -> denied("You cannot make opinion on deleted target", "opinion.create.deletedTarget") + else -> granted() + } } - override fun invoke(action: Any, context: ApplicationCall, subject: Any?): VoterResponseI { - if (!((action is Action) && - (subject is Opinion<*>? || subject is ArticleAuthI<*>))) return abstain() + fun > canView(subjects: SS, citizen: CitizenI?): VoterResponse = + canAll(subjects) { canView(it, citizen) } - val user = context.user - if (action == Action.CREATE) { - if (user == null) return denied("You must be connected to make an opinion", "opinion.create.notConnected") - if (subject is ArticleAuthI<*> && !subject.isDeleted()) return granted() - if (subject is Opinion<*> && subject.createdBy.user.id == user.id) return granted() + fun canView(subject: S, citizen: CitizenI?): VoterResponse = granted() - throw NoSubjectDefinedException(action) - } - - if (action == Action.VIEW) { - return if (subject is Opinion<*> || subject is ArticleForView) granted() else throw NoSubjectDefinedException(action) - } - - if (action == Action.DELETE) { - if (user == null) return denied("You must be connected to delete opinion", "opinion.delete.notConnected") - if (subject !is Opinion<*>) throw NoSubjectDefinedException(action) - return if (subject.createdBy.user.id == user.id) granted() else denied("You can only delete your opinions", "opinion.delete.notYours") - } - - if (action is Action) { - throw NoRuleDefinedException(action) - } - - return abstain() + fun canDelete(subject: S, citizen: CitizenI?): VoterResponse where S : EntityCreatedBy, S : OpinionI = when { + citizen == null -> denied("You must be connected to delete opinion", "opinion.delete.notConnected") + subject.createdBy.id != citizen.id -> denied("You can only delete your opinions", "opinion.delete.notYours") + else -> granted() } } diff --git a/src/test/kotlin/steps/OpinionSteps.kt b/src/test/kotlin/steps/OpinionSteps.kt index e219b3b..23d207a 100644 --- a/src/test/kotlin/steps/OpinionSteps.kt +++ b/src/test/kotlin/steps/OpinionSteps.kt @@ -77,6 +77,6 @@ class OpinionSteps : En, KoinTest { } ?: error("You must provide the 'article' parameter"), createdBy = get().findByUsername(username) ?: error("Citizen not exist") ) - get().updateOpinions(opinion.choice, opinion.createdBy, opinion.target) + get().updateOpinions(opinion) } } \ No newline at end of file diff --git a/src/test/kotlin/unit/voter/OpinionVoterTest.kt b/src/test/kotlin/unit/voter/OpinionVoterTest.kt index 43f78ea..6f2192f 100644 --- a/src/test/kotlin/unit/voter/OpinionVoterTest.kt +++ b/src/test/kotlin/unit/voter/OpinionVoterTest.kt @@ -10,18 +10,16 @@ import fr.dcproject.component.citizen.CitizenI import fr.dcproject.entity.Opinion import fr.dcproject.entity.OpinionChoice import fr.dcproject.security.voter.OpinionVoter -import fr.dcproject.voter.NoSubjectDefinedException +import fr.dcproject.voter.Vote.DENIED +import fr.dcproject.voter.Vote.GRANTED import fr.ktorVoter.* import io.ktor.application.* -import io.mockk.every -import io.mockk.mockk import io.mockk.mockkStatic import org.amshove.kluent.`should be` import org.joda.time.DateTime import org.junit.jupiter.api.Tag import org.junit.jupiter.api.Test import org.junit.jupiter.api.TestInstance -import org.junit.jupiter.api.assertThrows import org.junit.jupiter.api.parallel.Execution import org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT import java.util.* @@ -83,89 +81,51 @@ internal class OpinionVoterTest { } @Test - fun `support opinion`(): Unit = OpinionVoter().run { - val p = object : ActionI {} - mockk { - every { user } returns tesla.user - }.let { - this(OpinionVoter.Action.VIEW, it, opinion1).vote `should be` Vote.GRANTED - this(OpinionVoter.Action.VIEW, it, article1).vote `should be` Vote.GRANTED - this(OpinionVoter.Action.VIEW, it, einstein).vote `should be` Vote.ABSTAIN - this(p, it, opinion1).vote `should be` Vote.ABSTAIN - } + fun `can be view the opinion`() { + OpinionVoter() + .canView(opinion1, tesla) + .vote `should be` GRANTED } @Test - fun `can be view the opinion`(): Unit = listOf(OpinionVoter()).run { - mockk { - every { user } returns tesla.user - }.let { - can(OpinionVoter.Action.VIEW, it, opinion1) `should be` true - } + fun `can be view the opinion list`() { + OpinionVoter() + .canView(listOf(opinion1), tesla) + .vote `should be` GRANTED } @Test - fun `can be not view the opinion if is null`(): Unit = listOf(OpinionVoter()).run { - mockk { - every { user } returns tesla.user - }.let { - assertThrows { - assertCan(OpinionVoter.Action.VIEW, it, null) - } - } + fun `can be opinion an article`() { + OpinionVoter() + .canCreate(opinion1, tesla) + .vote `should be` GRANTED } @Test - fun `can be view the opinion list`(): Unit = listOf(OpinionVoter()).run { - mockk { - every { user } returns tesla.user - }.let { - canAll(OpinionVoter.Action.VIEW, it, listOf(opinion1)) `should be` true - } + fun `can not be opinion if not connected`() { + OpinionVoter() + .canCreate(opinion1, null) + .vote `should be` DENIED } @Test - fun `can be opinion an article`(): Unit = listOf(OpinionVoter()).run { - mockk { - every { user } returns tesla.user - }.let { - can(OpinionVoter.Action.CREATE, it, opinion1) `should be` true - } + fun `can be remove opinion`() { + OpinionVoter() + .canDelete(opinion1, tesla) + .vote `should be` GRANTED } @Test - fun `can not be opinion if not connected`() = listOf(OpinionVoter()).run { - mockk { - every { user } returns null - }.let { - can(OpinionVoter.Action.CREATE, it, opinion1) `should be` false - } + fun `can not be remove opinion if not connected`() { + OpinionVoter() + .canDelete(opinion1, null) + .vote `should be` DENIED } @Test - fun `can be remove opinion`(): Unit = listOf(OpinionVoter()).run { - mockk { - every { user } returns tesla.user - }.let { - can(OpinionVoter.Action.DELETE, it, opinion1) `should be` true - } - } - - @Test - fun `can not be remove opinion if not connected`(): Unit = listOf(OpinionVoter()).run { - mockk { - every { user } returns null - }.let { - can(OpinionVoter.Action.DELETE, it, opinion1) `should be` false - } - } - - @Test - fun `can not be remove opinion of other user`(): Unit = listOf(OpinionVoter()).run { - mockk { - every { user } returns einstein.user - }.let { - can(OpinionVoter.Action.DELETE, it, opinion1) `should be` false - } + fun `can not be remove opinion of other user`() { + OpinionVoter() + .canDelete(opinion1, einstein) + .vote `should be` DENIED } } \ No newline at end of file diff --git a/src/test/resources/feature/opinion.feature b/src/test/resources/feature/opinion.feature index 070a290..eb0bc57 100644 --- a/src/test/resources/feature/opinion.feature +++ b/src/test/resources/feature/opinion.feature @@ -24,7 +24,6 @@ Feature: Opinion | id | 9226c1a3-8091-c3fa-7d0d-c2e98c9bee7b | | createdBy | Isaac Newton | And I have an opinion choice "Opinion4" with ID "0f4f1721-3136-44f1-9f31-1459f3317b15" - And I have an opinion "Opinion4" on article "9226c1a3-8091-c3fa-7d0d-c2e98c9bee7b" created by Isaac Newton with ID "74e93e12-556b-4399-95a6-04f93a4dd66c" When I send a PUT request to "/articles/9226c1a3-8091-c3fa-7d0d-c2e98c9bee7b/opinions" with body: """ {