From fdb1d3404c5cd4caa9ae0ca7188d2dc3fe8e7fd3 Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Tue, 8 Oct 2019 14:15:08 +0300 Subject: [PATCH] FEATURE: Add site setting to show more detailed 404 errors. (#8014) If the setting is turned on, then the user will receive information about the subject: if it was deleted or requires some special access to a group (only if the group is public). Otherwise, the user will receive a generic #404 error message. For now, this change affects only the topics and categories controller. This commit also tries to refactor some of the code related to error handling. To make error pages more consistent (design-wise), the actual error page will be rendered server-side. --- .../discourse/controllers/topic.js.es6 | 42 +- .../discourse/models/post-stream.js.es6 | 27 +- .../javascripts/discourse/models/topic.js.es6 | 5 +- .../routes/build-category-route.js.es6 | 12 + .../discourse/templates/discovery.hbs | 52 +- .../javascripts/discourse/templates/topic.hbs | 646 +++++++++--------- .../stylesheets/common/base/not-found.scss | 11 +- app/controllers/application_controller.rb | 144 ++-- app/controllers/categories_controller.rb | 13 +- app/controllers/exceptions_controller.rb | 9 +- app/controllers/list_controller.rb | 8 +- app/controllers/topics_controller.rb | 80 ++- app/controllers/users_controller.rb | 2 - app/models/category.rb | 9 + .../hidden_topic_view_serializer.rb | 15 - app/views/exceptions/not_found.html.erb | 10 +- config/locales/server.en.yml | 7 + config/site_settings.yml | 1 + lib/discourse.rb | 16 +- lib/guardian/topic_guardian.rb | 4 - spec/requests/application_controller_spec.rb | 8 +- spec/requests/topics_controller_spec.rb | 303 +++++--- .../acceptance/topic-anonymous-test.js.es6 | 5 +- 23 files changed, 769 insertions(+), 660 deletions(-) delete mode 100644 app/serializers/hidden_topic_view_serializer.rb diff --git a/app/assets/javascripts/discourse/controllers/topic.js.es6 b/app/assets/javascripts/discourse/controllers/topic.js.es6 index ce14d2903a..f459e4bcec 100644 --- a/app/assets/javascripts/discourse/controllers/topic.js.es6 +++ b/app/assets/javascripts/discourse/controllers/topic.js.es6 @@ -945,46 +945,6 @@ export default Ember.Controller.extend(bufferedProperty("model"), { } }, - joinGroup() { - const groupId = this.get("model.group.id"); - if (groupId) { - if (this.get("model.group.allow_membership_requests")) { - const groupName = this.get("model.group.name"); - return ajax(`/groups/${groupName}/request_membership`, { - type: "POST", - data: { - topic_id: this.get("model.id") - } - }) - .then(() => { - bootbox.alert( - I18n.t("topic.group_request_sent", { - group_name: this.get("model.group.full_name") - }), - () => - this.previousURL - ? DiscourseURL.routeTo(this.previousURL) - : DiscourseURL.routeTo("/") - ); - }) - .catch(popupAjaxError); - } else { - const topic = this.model; - return ajax(`/groups/${groupId}/members`, { - type: "PUT", - data: { user_id: this.get("currentUser.id") } - }) - .then(() => - topic.reload().then(() => { - topic.set("view_hidden", false); - topic.postStream.refresh(); - }) - ) - .catch(popupAjaxError); - } - } - }, - replyAsNewTopic(post, quotedText) { const composerController = this.composer; @@ -1182,7 +1142,7 @@ export default Ember.Controller.extend(bufferedProperty("model"), { } }, - hasError: Ember.computed.or("model.notFoundHtml", "model.message"), + hasError: Ember.computed.or("model.errorHtml", "model.errorMessage"), noErrorYet: Ember.computed.not("hasError"), categories: Ember.computed.alias("site.categoriesList"), diff --git a/app/assets/javascripts/discourse/models/post-stream.js.es6 b/app/assets/javascripts/discourse/models/post-stream.js.es6 index 346226b9eb..a3b42bcaeb 100644 --- a/app/assets/javascripts/discourse/models/post-stream.js.es6 +++ b/app/assets/javascripts/discourse/models/post-stream.js.es6 @@ -1067,31 +1067,16 @@ export default RestModel.extend({ // Handles an error loading a topic based on a HTTP status code. Updates // the text to the correct values. errorLoading(result) { - const status = result.jqXHR.status; - const topic = this.topic; this.set("loadingFilter", false); topic.set("errorLoading", true); - // If the result was 404 the post is not found - // If it was 410 the post is deleted and the user should not see it - if (status === 404 || status === 410) { - topic.set("notFoundHtml", result.jqXHR.responseText); - return; + const json = result.jqXHR.responseJSON; + if (json && json.extras && json.extras.html) { + topic.set("errorHtml", json.extras.html); + } else { + topic.set("errorMessage", I18n.t("topic.server_error.description")); + topic.set("noRetry", result.jqXHR.status === 403); } - - // If the result is 403 it means invalid access - if (status === 403) { - topic.set("noRetry", true); - if (Discourse.User.current()) { - topic.set("message", I18n.t("topic.invalid_access.description")); - } else { - topic.set("message", I18n.t("topic.invalid_access.login_required")); - } - return; - } - - // Otherwise supply a generic error message - topic.set("message", I18n.t("topic.server_error.description")); } }); diff --git a/app/assets/javascripts/discourse/models/topic.js.es6 b/app/assets/javascripts/discourse/models/topic.js.es6 index d44afe930e..1af3697106 100644 --- a/app/assets/javascripts/discourse/models/topic.js.es6 +++ b/app/assets/javascripts/discourse/models/topic.js.es6 @@ -17,16 +17,15 @@ import { } from "ember-addons/ember-computed-decorators"; export function loadTopicView(topic, args) { - const topicId = topic.get("id"); const data = _.merge({}, args); - const url = `${Discourse.getURL("/t/")}${topicId}`; + const url = `${Discourse.getURL("/t/")}${topic.id}`; const jsonUrl = (data.nearPost ? `${url}/${data.nearPost}` : url) + ".json"; delete data.nearPost; delete data.__type; delete data.store; - return PreloadStore.getAndRemove(`topic_${topicId}`, () => + return PreloadStore.getAndRemove(`topic_${topic.id}`, () => ajax(jsonUrl, { data }) ).then(json => { topic.updateFromJson(json); diff --git a/app/assets/javascripts/discourse/routes/build-category-route.js.es6 b/app/assets/javascripts/discourse/routes/build-category-route.js.es6 index c1db98d777..aadf72eb35 100644 --- a/app/assets/javascripts/discourse/routes/build-category-route.js.es6 +++ b/app/assets/javascripts/discourse/routes/build-category-route.js.es6 @@ -198,6 +198,18 @@ export default (filterArg, params) => { }, actions: { + error(err) { + const json = err.jqXHR.responseJSON; + if (json && json.extras && json.extras.html) { + this.controllerFor("discovery").set( + "errorHtml", + err.jqXHR.responseJSON.extras.html + ); + } else { + this.replaceWith("exception"); + } + }, + setNotification(notification_level) { this.currentModel.setNotification(notification_level); }, diff --git a/app/assets/javascripts/discourse/templates/discovery.hbs b/app/assets/javascripts/discourse/templates/discovery.hbs index c6242e9e5b..6801ab3f55 100644 --- a/app/assets/javascripts/discourse/templates/discovery.hbs +++ b/app/assets/javascripts/discourse/templates/discovery.hbs @@ -1,32 +1,36 @@ -
- {{discourse-banner user=currentUser banner=site.banner}} -
- -
+{{#if errorHtml}} + {{{errorHtml}}} +{{else}}
- {{outlet "navigation-bar"}} + {{discourse-banner user=currentUser banner=site.banner}}
-
-{{conditional-loading-spinner condition=loading}} +
+
+ {{outlet "navigation-bar"}} +
+
-
-
-
-
- {{outlet "header-list-container"}} + {{conditional-loading-spinner condition=loading}} + +
+
+
+
+ {{outlet "header-list-container"}} +
+
+
+
+
+
+ {{plugin-outlet name="discovery-list-container-top" + args=(hash category=category listLoading=loading)}} + {{outlet "list-container"}} +
-
-
-
- {{plugin-outlet name="discovery-list-container-top" - args=(hash category=category listLoading=loading)}} - {{outlet "list-container"}} -
-
-
-
-{{plugin-outlet name="discovery-below"}} + {{plugin-outlet name="discovery-below"}} +{{/if}} diff --git a/app/assets/javascripts/discourse/templates/topic.hbs b/app/assets/javascripts/discourse/templates/topic.hbs index 07691dfa0f..9d77e4a4b5 100644 --- a/app/assets/javascripts/discourse/templates/topic.hbs +++ b/app/assets/javascripts/discourse/templates/topic.hbs @@ -1,105 +1,154 @@ {{#discourse-topic multiSelect=multiSelect enteredAt=enteredAt topic=model hasScrolled=hasScrolled}} - {{#if model.view_hidden}} - {{topic-join-group-notice model=model action=(action "joinGroup")}} - {{else}} - {{#if model}} - {{add-category-tag-classes category=model.category tags=model.tags}} -
- {{discourse-banner user=currentUser banner=site.banner overlay=hasScrolled hide=model.errorLoading}} -
- {{/if}} + {{#if model}} + {{add-category-tag-classes category=model.category tags=model.tags}} +
+ {{discourse-banner user=currentUser banner=site.banner overlay=hasScrolled hide=model.errorLoading}} +
+ {{/if}} - {{#if showSharedDraftControls}} - {{shared-draft-controls topic=model}} - {{/if}} + {{#if showSharedDraftControls}} + {{shared-draft-controls topic=model}} + {{/if}} - {{plugin-outlet name="topic-above-post-stream" args=(hash model=model)}} + {{plugin-outlet name="topic-above-post-stream" args=(hash model=model)}} - {{#if model.postStream.loaded}} - {{#if model.postStream.firstPostPresent}} - {{#topic-title cancelled=(action "cancelEditingTopic") save=(action "finishedEditingTopic") model=model}} - {{#if editingTopic}} -
- {{#if model.isPrivateMessage}} - {{d-icon "envelope"}} + {{#if model.postStream.loaded}} + {{#if model.postStream.firstPostPresent}} + {{#topic-title cancelled=(action "cancelEditingTopic") save=(action "finishedEditingTopic") model=model}} + {{#if editingTopic}} +
+ {{#if model.isPrivateMessage}} + {{d-icon "envelope"}} + {{/if}} + {{text-field id="edit-title" value=buffered.title maxlength=siteSettings.max_topic_title_length autofocus="true"}} + + {{#if showCategoryChooser}} + {{category-chooser + class="small" + value=(unbound buffered.category_id) + onSelectAny=(action "topicCategoryChanged")}} {{/if}} - {{text-field id="edit-title" value=buffered.title maxlength=siteSettings.max_topic_title_length autofocus="true"}} - {{#if showCategoryChooser}} - {{category-chooser - class="small" - value=(unbound buffered.category_id) - onSelectAny=(action "topicCategoryChanged")}} - {{/if}} + {{#if canEditTags}} + {{mini-tag-chooser + filterable=true + tags=(unbound buffered.tags) + categoryId=(unbound buffered.category_id) + onChangeTags=(action "topicTagsChanged")}} + {{/if}} - {{#if canEditTags}} - {{mini-tag-chooser - filterable=true - tags=(unbound buffered.tags) - categoryId=(unbound buffered.category_id) - onChangeTags=(action "topicTagsChanged")}} - {{/if}} + {{plugin-outlet name="edit-topic" args=(hash model=model buffered=buffered)}} - {{plugin-outlet name="edit-topic" args=(hash model=model buffered=buffered)}} +
+ {{d-button action=(action "finishedEditingTopic") class="btn-primary submit-edit" icon="check"}} + {{d-button action=(action "cancelEditingTopic") class="btn-default cancel-edit" icon="times"}} -
- {{d-button action=(action "finishedEditingTopic") class="btn-primary submit-edit" icon="check"}} - {{d-button action=(action "cancelEditingTopic") class="btn-default cancel-edit" icon="times"}} - - {{#if canRemoveTopicFeaturedLink}} - - {{d-icon "times-circle"}} - {{featuredLinkDomain}} - - {{/if}} -
-
- - {{else}} -

- {{#unless model.is_warning}} - {{#if siteSettings.enable_personal_messages}} - {{#if model.isPrivateMessage}} - - {{d-icon "envelope"}} - - {{/if}} - {{else}} - {{#if model.isPrivateMessage}} - {{d-icon "envelope"}} - {{/if}} - {{/if}} - {{/unless}} - - {{#if model.details.loaded}} - {{topic-status topic=model}} - - {{{model.fancyTitle}}} + {{#if canRemoveTopicFeaturedLink}} + + {{d-icon "times-circle"}} + {{featuredLinkDomain}} {{/if}} +

+
- {{#if model.details.can_edit}} - {{d-icon "pencil-alt"}} + {{else}} +

+ {{#unless model.is_warning}} + {{#if siteSettings.enable_personal_messages}} + {{#if model.isPrivateMessage}} + + {{d-icon "envelope"}} + + {{/if}} + {{else}} + {{#if model.isPrivateMessage}} + {{d-icon "envelope"}} + {{/if}} {{/if}} -

+ {{/unless}} - {{topic-category topic=model class="topic-category"}} + {{#if model.details.loaded}} + {{topic-status topic=model}} + + {{{model.fancyTitle}}} + + {{/if}} + + {{#if model.details.can_edit}} + {{d-icon "pencil-alt"}} + {{/if}} + + + {{topic-category topic=model class="topic-category"}} + {{/if}} + {{/topic-title}} + {{/if}} + + +
+
+ {{partial "selected-posts"}} +
+ + {{#topic-navigation topic=model jumpToDate=(action "jumpToDate") jumpToIndex=(action "jumpToIndex") as |info|}} + {{#if info.renderTimeline}} + {{#if info.renderAdminMenuButton}} + {{topic-admin-menu-button + topic=model + fixed="true" + toggleMultiSelect=(action "toggleMultiSelect") + deleteTopic=(action "deleteTopic") + recoverTopic=(action "recoverTopic") + toggleClosed=(action "toggleClosed") + toggleArchived=(action "toggleArchived") + toggleVisibility=(action "toggleVisibility") + showTopicStatusUpdate=(route-action "showTopicStatusUpdate") + showFeatureTopic=(route-action "showFeatureTopic") + showChangeTimestamp=(route-action "showChangeTimestamp") + resetBumpDate=(action "resetBumpDate") + convertToPublicTopic=(action "convertToPublicTopic") + convertToPrivateMessage=(action "convertToPrivateMessage")}} {{/if}} - {{/topic-title}} - {{/if}} - -
-
- {{partial "selected-posts"}} -
- - {{#topic-navigation topic=model jumpToDate=(action "jumpToDate") jumpToIndex=(action "jumpToIndex") as |info|}} - {{#if info.renderTimeline}} + {{topic-timeline + topic=model + notificationLevel=model.details.notification_level + prevEvent=info.prevEvent + fullscreen=info.topicProgressExpanded + enteredIndex=enteredIndex + loading=model.postStream.loading + jumpToPost=(action "jumpToPost") + jumpTop=(action "jumpTop") + jumpBottom=(action "jumpBottom") + jumpEnd=(action "jumpEnd") + jumpToPostPrompt=(action "jumpToPostPrompt") + jumpToIndex=(action "jumpToIndex") + replyToPost=(action "replyToPost") + toggleMultiSelect=(action "toggleMultiSelect") + deleteTopic=(action "deleteTopic") + recoverTopic=(action "recoverTopic") + toggleClosed=(action "toggleClosed") + toggleArchived=(action "toggleArchived") + toggleVisibility=(action "toggleVisibility") + showTopicStatusUpdate=(route-action "showTopicStatusUpdate") + showFeatureTopic=(route-action "showFeatureTopic") + showChangeTimestamp=(route-action "showChangeTimestamp") + resetBumpDate=(action "resetBumpDate") + convertToPublicTopic=(action "convertToPublicTopic") + convertToPrivateMessage=(action "convertToPrivateMessage")}} + {{else}} + {{#topic-progress + prevEvent=info.prevEvent + topic=model + expanded=info.topicProgressExpanded + jumpToPost=(action "jumpToPost")}} + {{plugin-outlet name="before-topic-progress" args=(hash model=model jumpToPost=(action "jumpToPost"))}} {{#if info.renderAdminMenuButton}} {{topic-admin-menu-button topic=model - fixed="true" + openUpwards="true" + rightSide="true" toggleMultiSelect=(action "toggleMultiSelect") deleteTopic=(action "deleteTopic") recoverTopic=(action "recoverTopic") @@ -113,265 +162,212 @@ convertToPublicTopic=(action "convertToPublicTopic") convertToPrivateMessage=(action "convertToPrivateMessage")}} {{/if}} + {{/topic-progress}} + {{/if}} + {{/topic-navigation}} - {{topic-timeline - topic=model - notificationLevel=model.details.notification_level - prevEvent=info.prevEvent - fullscreen=info.topicProgressExpanded - enteredIndex=enteredIndex - loading=model.postStream.loading - jumpToPost=(action "jumpToPost") - jumpTop=(action "jumpTop") - jumpBottom=(action "jumpBottom") - jumpEnd=(action "jumpEnd") - jumpToPostPrompt=(action "jumpToPostPrompt") - jumpToIndex=(action "jumpToIndex") - replyToPost=(action "replyToPost") - toggleMultiSelect=(action "toggleMultiSelect") - deleteTopic=(action "deleteTopic") - recoverTopic=(action "recoverTopic") - toggleClosed=(action "toggleClosed") - toggleArchived=(action "toggleArchived") - toggleVisibility=(action "toggleVisibility") - showTopicStatusUpdate=(route-action "showTopicStatusUpdate") - showFeatureTopic=(route-action "showFeatureTopic") - showChangeTimestamp=(route-action "showChangeTimestamp") - resetBumpDate=(action "resetBumpDate") - convertToPublicTopic=(action "convertToPublicTopic") - convertToPrivateMessage=(action "convertToPrivateMessage")}} - {{else}} - {{#topic-progress - prevEvent=info.prevEvent - topic=model - expanded=info.topicProgressExpanded - jumpToPost=(action "jumpToPost")}} - {{plugin-outlet name="before-topic-progress" args=(hash model=model jumpToPost=(action "jumpToPost"))}} - {{#if info.renderAdminMenuButton}} - {{topic-admin-menu-button - topic=model - openUpwards="true" - rightSide="true" - toggleMultiSelect=(action "toggleMultiSelect") - deleteTopic=(action "deleteTopic") - recoverTopic=(action "recoverTopic") - toggleClosed=(action "toggleClosed") - toggleArchived=(action "toggleArchived") - toggleVisibility=(action "toggleVisibility") - showTopicStatusUpdate=(route-action "showTopicStatusUpdate") - showFeatureTopic=(route-action "showFeatureTopic") - showChangeTimestamp=(route-action "showChangeTimestamp") - resetBumpDate=(action "resetBumpDate") - convertToPublicTopic=(action "convertToPublicTopic") - convertToPrivateMessage=(action "convertToPrivateMessage")}} - {{/if}} - {{/topic-progress}} - {{/if}} - {{/topic-navigation}} +
+
-
-
+
+ {{conditional-loading-spinner condition=model.postStream.loadingAbove}} -
- {{conditional-loading-spinner condition=model.postStream.loadingAbove}} + {{plugin-outlet name="topic-above-posts" args=(hash model=model)}} - {{plugin-outlet name="topic-above-posts" args=(hash model=model)}} + {{#unless model.postStream.loadingFilter}} + {{scrolling-post-stream + posts=postsToRender + canCreatePost=model.details.can_create_post + multiSelect=multiSelect + selectedPostsCount=selectedPostsCount + selectedQuery=selectedQuery + gaps=model.postStream.gaps + showReadIndicator=model.show_read_indicator + showFlags=(action "showPostFlags") + editPost=(action "editPost") + showHistory=(route-action "showHistory") + showLogin=(route-action "showLogin") + showRawEmail=(route-action "showRawEmail") + deletePost=(action "deletePost") + recoverPost=(action "recoverPost") + expandHidden=(action "expandHidden") + newTopicAction=(action "replyAsNewTopic") + toggleBookmark=(action "toggleBookmark") + togglePostType=(action "togglePostType") + rebakePost=(action "rebakePost") + changePostOwner=(action "changePostOwner") + grantBadge=(action "grantBadge") + addNotice=(action "addNotice") + removeNotice=(action "removeNotice") + lockPost=(action "lockPost") + unlockPost=(action "unlockPost") + unhidePost=(action "unhidePost") + replyToPost=(action "replyToPost") + toggleWiki=(action "toggleWiki") + toggleSummary=(action "toggleSummary") + removeAllowedUser=(action "removeAllowedUser") + removeAllowedGroup=(action "removeAllowedGroup") + topVisibleChanged=(action "topVisibleChanged") + currentPostChanged=(action "currentPostChanged") + currentPostScrolled=(action "currentPostScrolled") + bottomVisibleChanged=(action "bottomVisibleChanged") + togglePostSelection=(action "togglePostSelection") + selectReplies=(action "selectReplies") + selectBelow=(action "selectBelow") + fillGapBefore=(action "fillGapBefore") + fillGapAfter=(action "fillGapAfter") + showInvite=(route-action "showInvite")}} + {{/unless}} - {{#unless model.postStream.loadingFilter}} - {{scrolling-post-stream - posts=postsToRender - canCreatePost=model.details.can_create_post - multiSelect=multiSelect - selectedPostsCount=selectedPostsCount - selectedQuery=selectedQuery - gaps=model.postStream.gaps - showReadIndicator=model.show_read_indicator - showFlags=(action "showPostFlags") - editPost=(action "editPost") - showHistory=(route-action "showHistory") - showLogin=(route-action "showLogin") - showRawEmail=(route-action "showRawEmail") - deletePost=(action "deletePost") - recoverPost=(action "recoverPost") - expandHidden=(action "expandHidden") - newTopicAction=(action "replyAsNewTopic") - toggleBookmark=(action "toggleBookmark") - togglePostType=(action "togglePostType") - rebakePost=(action "rebakePost") - changePostOwner=(action "changePostOwner") - grantBadge=(action "grantBadge") - addNotice=(action "addNotice") - removeNotice=(action "removeNotice") - lockPost=(action "lockPost") - unlockPost=(action "unlockPost") - unhidePost=(action "unhidePost") - replyToPost=(action "replyToPost") - toggleWiki=(action "toggleWiki") - toggleSummary=(action "toggleSummary") - removeAllowedUser=(action "removeAllowedUser") - removeAllowedGroup=(action "removeAllowedGroup") - topVisibleChanged=(action "topVisibleChanged") - currentPostChanged=(action "currentPostChanged") - currentPostScrolled=(action "currentPostScrolled") - bottomVisibleChanged=(action "bottomVisibleChanged") - togglePostSelection=(action "togglePostSelection") - selectReplies=(action "selectReplies") - selectBelow=(action "selectBelow") - fillGapBefore=(action "fillGapBefore") - fillGapAfter=(action "fillGapAfter") - showInvite=(route-action "showInvite")}} - {{/unless}} + {{conditional-loading-spinner condition=model.postStream.loadingBelow}} +
+
- {{conditional-loading-spinner condition=model.postStream.loadingBelow}} -
-
+ {{#conditional-loading-spinner condition=model.postStream.loadingFilter}} + {{#if loadedAllPosts}} - {{#conditional-loading-spinner condition=model.postStream.loadingFilter}} - {{#if loadedAllPosts}} - - {{#if model.pending_posts}} -
- {{#each model.pending_posts as |pending|}} -
-
- - {{i18n "review.awaiting_approval"}} - - - {{age-with-tooltip pending.created_at}} - -
-
- {{reviewable-created-by user=currentUser tagName=''}} -
- {{reviewable-created-by-name user=currentUser tagName=''}} -
{{cook-text pending.raw}}
-
-
-
- {{d-button - class="btn-danger" - label="review.delete" - icon="trash-alt" - action=(action "deletePending" pending) }} + {{#if model.pending_posts}} +
+ {{#each model.pending_posts as |pending|}} +
+
+ + {{i18n "review.awaiting_approval"}} + + + {{age-with-tooltip pending.created_at}} + +
+
+ {{reviewable-created-by user=currentUser tagName=''}} +
+ {{reviewable-created-by-name user=currentUser tagName=''}} +
{{cook-text pending.raw}}
- {{/each}} -
- {{/if}} - - {{#if model.queued_posts_count}} -
-
- {{{i18n "review.topic_has_pending" count=model.queued_posts_count}}} +
+ {{d-button + class="btn-danger" + label="review.delete" + icon="trash-alt" + action=(action "deletePending" pending) }} +
- - {{#link-to 'review' (query-params topic_id=model.id type="ReviewableQueuedPost" status="pending")}} - {{i18n "review.view_pending"}} - {{/link-to}} -
- {{/if}} - - {{#if model.private_topic_timer.execute_at}} - {{topic-timer-info - topicClosed=model.closed - statusType=model.private_topic_timer.status_type - executeAt=model.private_topic_timer.execute_at - duration=model.private_topic_timer.duration - removeTopicTimer=(action "removeTopicTimer" model.private_topic_timer.status_type "private_topic_timer")}} - {{/if}} - - {{topic-timer-info - topicClosed=model.closed - statusType=model.topic_timer.status_type - executeAt=model.topic_timer.execute_at - basedOnLastPost=model.topic_timer.based_on_last_post - duration=model.topic_timer.duration - categoryId=model.topic_timer.category_id - removeTopicTimer=(action "removeTopicTimer" model.topic_timer.status_type "topic_timer")}} - - {{#if session.showSignupCta}} - {{! replace "Log In to Reply" with the infobox }} - {{signup-cta}} - {{else}} - {{#if currentUser}} - {{plugin-outlet name="topic-above-footer-buttons" args=(hash model=model)}} - - {{topic-footer-buttons - topic=model - toggleMultiSelect=(action "toggleMultiSelect") - deleteTopic=(action "deleteTopic") - recoverTopic=(action "recoverTopic") - toggleClosed=(action "toggleClosed") - toggleArchived=(action "toggleArchived") - toggleVisibility=(action "toggleVisibility") - showTopicStatusUpdate=(route-action "showTopicStatusUpdate") - showFeatureTopic=(route-action "showFeatureTopic") - showChangeTimestamp=(route-action "showChangeTimestamp") - resetBumpDate=(action "resetBumpDate") - convertToPublicTopic=(action "convertToPublicTopic") - convertToPrivateMessage=(action "convertToPrivateMessage") - toggleBookmark=(action "toggleBookmark") - showFlagTopic=(route-action "showFlagTopic") - toggleArchiveMessage=(action "toggleArchiveMessage") - editFirstPost=(action "editFirstPost") - deferTopic=(action "deferTopic") - replyToPost=(action "replyToPost")}} - {{else}} - - {{/if}} - {{/if}} - - {{#if showSelectedPostsAtBottom}} -
- {{partial "selected-posts"}} -
- {{/if}} - - {{plugin-outlet name="topic-above-suggested" args=(hash model=model)}} -
- {{#if model.relatedMessages.length}} - {{related-messages topic=model}} - {{/if}} - {{#if model.suggestedTopics.length}} - {{suggested-topics topic=model}} - {{/if}} + {{/each}}
{{/if}} - {{/conditional-loading-spinner}} -
-
+ {{#if model.queued_posts_count}} +
+
+ {{{i18n "review.topic_has_pending" count=model.queued_posts_count}}} +
-
- {{else}} -
- {{#conditional-loading-spinner condition=noErrorYet}} - {{#if model.notFoundHtml}} -
{{{model.notFoundHtml}}}
- {{else}} -
-
{{model.message}}
- {{#if model.noRetry}} - {{#unless currentUser}} - {{d-button action=(route-action "showLogin") class="btn-primary topic-retry" icon="user" label="log_in"}} - {{/unless}} - {{else}} - {{d-button action=(action "retryLoading") class="btn-primary topic-retry" icon="sync" label="errors.buttons.again"}} + {{#link-to 'review' (query-params topic_id=model.id type="ReviewableQueuedPost" status="pending")}} + {{i18n "review.view_pending"}} + {{/link-to}} +
{{/if}} -
- {{conditional-loading-spinner condition=retrying}} - {{/if}} - {{/conditional-loading-spinner}} + + {{#if model.private_topic_timer.execute_at}} + {{topic-timer-info + topicClosed=model.closed + statusType=model.private_topic_timer.status_type + executeAt=model.private_topic_timer.execute_at + duration=model.private_topic_timer.duration + removeTopicTimer=(action "removeTopicTimer" model.private_topic_timer.status_type "private_topic_timer")}} + {{/if}} + + {{topic-timer-info + topicClosed=model.closed + statusType=model.topic_timer.status_type + executeAt=model.topic_timer.execute_at + basedOnLastPost=model.topic_timer.based_on_last_post + duration=model.topic_timer.duration + categoryId=model.topic_timer.category_id + removeTopicTimer=(action "removeTopicTimer" model.topic_timer.status_type "topic_timer")}} + + {{#if session.showSignupCta}} + {{! replace "Log In to Reply" with the infobox }} + {{signup-cta}} + {{else}} + {{#if currentUser}} + {{plugin-outlet name="topic-above-footer-buttons" args=(hash model=model)}} + + {{topic-footer-buttons + topic=model + toggleMultiSelect=(action "toggleMultiSelect") + deleteTopic=(action "deleteTopic") + recoverTopic=(action "recoverTopic") + toggleClosed=(action "toggleClosed") + toggleArchived=(action "toggleArchived") + toggleVisibility=(action "toggleVisibility") + showTopicStatusUpdate=(route-action "showTopicStatusUpdate") + showFeatureTopic=(route-action "showFeatureTopic") + showChangeTimestamp=(route-action "showChangeTimestamp") + resetBumpDate=(action "resetBumpDate") + convertToPublicTopic=(action "convertToPublicTopic") + convertToPrivateMessage=(action "convertToPrivateMessage") + toggleBookmark=(action "toggleBookmark") + showFlagTopic=(route-action "showFlagTopic") + toggleArchiveMessage=(action "toggleArchiveMessage") + editFirstPost=(action "editFirstPost") + deferTopic=(action "deferTopic") + replyToPost=(action "replyToPost")}} + {{else}} + + {{/if}} + {{/if}} + + {{#if showSelectedPostsAtBottom}} +
+ {{partial "selected-posts"}} +
+ {{/if}} + + {{plugin-outlet name="topic-above-suggested" args=(hash model=model)}} +
+ {{#if model.relatedMessages.length}} + {{related-messages topic=model}} + {{/if}} + {{#if model.suggestedTopics.length}} + {{suggested-topics topic=model}} + {{/if}} +
+ {{/if}} + {{/conditional-loading-spinner}} + +
- {{/if}} - {{share-popup topic=model replyAsNewTopic=(action "replyAsNewTopic")}} +
+ {{else}} +
+ {{#conditional-loading-spinner condition=noErrorYet}} + {{#if model.errorHtml}} +
{{{model.errorHtml}}}
+ {{else}} +
+
{{model.errorMessage}}
+ {{#if model.noRetry}} + {{#unless currentUser}} + {{d-button action=(route-action "showLogin") class="btn-primary topic-retry" icon="user" label="log_in"}} + {{/unless}} + {{else}} + {{d-button action=(action "retryLoading") class="btn-primary topic-retry" icon="sync" label="errors.buttons.again"}} + {{/if}} +
+ {{conditional-loading-spinner condition=retrying}} + {{/if}} + {{/conditional-loading-spinner}} +
+ {{/if}} - {{#if embedQuoteButton}} - {{quote-button quoteState=quoteState selectText=(action "selectText")}} - {{/if}} + {{share-popup topic=model replyAsNewTopic=(action "replyAsNewTopic")}} + + {{#if embedQuoteButton}} + {{quote-button quoteState=quoteState selectText=(action "selectText")}} {{/if}} {{/discourse-topic}} diff --git a/app/assets/stylesheets/common/base/not-found.scss b/app/assets/stylesheets/common/base/not-found.scss index 1cde53d2eb..da46e2151f 100644 --- a/app/assets/stylesheets/common/base/not-found.scss +++ b/app/assets/stylesheets/common/base/not-found.scss @@ -1,12 +1,13 @@ // Page not found styles -h1.page-not-found { - font-size: $font-up-5; - line-height: $line-height-medium; -} - .page-not-found { margin: 0 0 40px 0; + + h1.title { + font-size: $font-up-5; + line-height: $line-height-medium; + } + &-search { margin-top: 20px; } diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5d5db79982..26a1bdf175 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -98,15 +98,59 @@ class ApplicationController < ActionController::Base use_crawler_layout? ? 'crawler' : 'application' end - # Some exceptions class RenderEmpty < StandardError; end + class PluginDisabled < StandardError; end - # Render nothing rescue_from RenderEmpty do render 'default/empty' end - def render_rate_limit_error(e) + rescue_from ArgumentError do |e| + if e.message == "string contains null byte" + raise Discourse::InvalidParameters, e.message + else + raise e + end + end + + rescue_from PG::ReadOnlySqlTransaction do |e| + Discourse.received_postgres_readonly! + Rails.logger.error("#{e.class} #{e.message}: #{e.backtrace.join("\n")}") + raise Discourse::ReadOnly + end + + rescue_from ActionController::ParameterMissing do |e| + render_json_error e.message, status: 400 + end + + rescue_from ActionController::RoutingError, PluginDisabled do + rescue_discourse_actions(:not_found, 404) + end + + # Handles requests for giant IDs that throw pg exceptions + rescue_from ActiveModel::RangeError do |e| + if e.message =~ /ActiveModel::Type::Integer/ + rescue_discourse_actions(:not_found, 404) + else + raise e + end + end + + rescue_from ActiveRecord::RecordInvalid do |e| + if request.format && request.format.json? + render_json_error e, type: :record_invalid, status: 422 + else + raise e + end + end + + rescue_from ActiveRecord::StatementInvalid do |e| + Discourse.reset_active_record_cache_if_needed(e) + raise e + end + + # If they hit the rate limiter + rescue_from RateLimiter::LimitExceeded do |e| retry_time_in_seconds = e&.available_in render_json_error( @@ -118,25 +162,6 @@ class ApplicationController < ActionController::Base ) end - rescue_from ActiveRecord::RecordInvalid do |e| - if request.format && request.format.json? - render_json_error e, type: :record_invalid, status: 422 - else - raise e - end - end - - # If they hit the rate limiter - rescue_from RateLimiter::LimitExceeded do |e| - render_rate_limit_error(e) - end - - rescue_from PG::ReadOnlySqlTransaction do |e| - Discourse.received_postgres_readonly! - Rails.logger.error("#{e.class} #{e.message}: #{e.backtrace.join("\n")}") - raise Discourse::ReadOnly - end - rescue_from Discourse::NotLoggedIn do |e| if (request.format && request.format.json?) || request.xhr? || !request.get? rescue_discourse_actions(:not_logged_in, 403, include_ember: true) @@ -145,14 +170,6 @@ class ApplicationController < ActionController::Base end end - rescue_from ArgumentError do |e| - if e.message == "string contains null byte" - raise Discourse::InvalidParameters, e.message - else - raise e - end - end - rescue_from Discourse::InvalidParameters do |e| message = I18n.t('invalid_params', message: e.message) if (request.format && request.format.json?) || request.xhr? || !request.get? @@ -162,45 +179,27 @@ class ApplicationController < ActionController::Base end end - rescue_from ActiveRecord::StatementInvalid do |e| - Discourse.reset_active_record_cache_if_needed(e) - raise e - end - - class PluginDisabled < StandardError; end - - # Handles requests for giant IDs that throw pg exceptions - rescue_from ActiveModel::RangeError do |e| - if e.message =~ /ActiveModel::Type::Integer/ - rescue_discourse_actions(:not_found, 404) - else - raise e - end - end - rescue_from Discourse::NotFound do |e| rescue_discourse_actions( :not_found, e.status, check_permalinks: e.check_permalinks, - original_path: e.original_path + original_path: e.original_path, + custom_message: e.custom_message ) end - rescue_from PluginDisabled, ActionController::RoutingError do - rescue_discourse_actions(:not_found, 404) - end - rescue_from Discourse::InvalidAccess do |e| - if e.opts[:delete_cookie].present? cookies.delete(e.opts[:delete_cookie]) end + rescue_discourse_actions( :invalid_access, 403, include_ember: true, - custom_message: e.custom_message + custom_message: e.custom_message, + group: e.group ) end @@ -208,10 +207,6 @@ class ApplicationController < ActionController::Base render_json_error I18n.t('read_only_mode_enabled'), type: :read_only, status: 503 end - rescue_from ActionController::ParameterMissing do |e| - render_json_error e.message, status: 400 - end - def redirect_with_client_support(url, options) if request.xhr? response.headers['Discourse-Xhr-Redirect'] = 'true' @@ -241,18 +236,21 @@ class ApplicationController < ActionController::Base end message = opts[:custom_message_translated] || I18n.t(opts[:custom_message] || type) + error_page_opts = { + title: opts[:custom_message_translated] || I18n.t(opts[:custom_message] || "page_not_found.title"), + status: status_code, + group: opts[:group] + } if show_json_errors - # HACK: do not use render_json_error for topics#show - if request.params[:controller] == 'topics' && request.params[:action] == 'show' - return render( - status: status_code, - layout: false, - plain: (status_code == 404 || status_code == 410) ? build_not_found_page(status_code) : message - ) + opts = { type: type, status: status_code } + + # Include error in HTML format for topics#show. + if (request.params[:controller] == 'topics' && request.params[:action] == 'show') || (request.params[:controller] == 'categories' && request.params[:action] == 'find_by_slug') + opts[:extras] = { html: build_not_found_page(error_page_opts) } end - render_json_error message, type: type, status: status_code + render_json_error message, opts else begin # 404 pages won't have the session and theme_keys without these: @@ -262,7 +260,8 @@ class ApplicationController < ActionController::Base return render plain: message, status: status_code end - render html: build_not_found_page(status_code, opts[:include_ember] ? 'application' : 'no_ember') + error_page_opts[:layout] = opts[:include_ember] ? 'application' : 'no_ember' + render html: build_not_found_page(error_page_opts) end end @@ -754,13 +753,13 @@ class ApplicationController < ActionController::Base raise Discourse::ReadOnly.new if !(request.get? || request.head?) && @readonly_mode end - def build_not_found_page(status = 404, layout = false) + def build_not_found_page(opts = {}) if SiteSetting.bootstrap_error_pages? preload_json - layout = 'application' if layout == 'no_ember' + opts[:layout] = 'application' if opts[:layout] == 'no_ember' end - if !SiteSetting.login_required? || current_user + if !SiteSetting.login_required? || (current_user rescue false) key = "page_not_found_topics" if @topics_partial = $redis.get(key) @topics_partial = @topics_partial.html_safe @@ -774,9 +773,12 @@ class ApplicationController < ActionController::Base end @container_class = "wrap not-found-container" - @slug = (params[:slug].presence || params[:id].presence || "").tr('-', '') + @title = opts[:title] || I18n.t("page_not_found.title") + @group = opts[:group] @hide_search = true if SiteSetting.login_required - render_to_string status: status, layout: layout, formats: [:html], template: '/exceptions/not_found' + @slug = (params[:slug].presence || params[:id].presence || "").tr('-', ' ') + + render_to_string status: opts[:status], layout: opts[:layout], formats: [:html], template: '/exceptions/not_found' end def is_asset_path diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 47fbd2ab8f..6f60b09cff 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -206,7 +206,18 @@ class CategoriesController < ApplicationController def find_by_slug params.require(:category_slug) @category = Category.find_by_slug(params[:category_slug], params[:parent_category_slug]) - guardian.ensure_can_see!(@category) + if !guardian.can_see?(@category) + if SiteSetting.detailed_404 && group = @category.access_category_via_group + raise Discourse::InvalidAccess.new( + 'not in group', + @category, + custom_message: 'not_in_group.title_category', + group: group + ) + else + raise Discourse::NotFound + end + end @category.permission = CategoryGroup.permission_types[:full] if Category.topic_create_allowed(guardian).where(id: @category.id).exists? render_serialized(@category, CategorySerializer) diff --git a/app/controllers/exceptions_controller.rb b/app/controllers/exceptions_controller.rb index 225c383d6c..2cbfeeed02 100644 --- a/app/controllers/exceptions_controller.rb +++ b/app/controllers/exceptions_controller.rb @@ -2,7 +2,6 @@ class ExceptionsController < ApplicationController skip_before_action :check_xhr, :preload_json - before_action :hide_search def not_found # centralize all rendering of 404 into app controller @@ -11,13 +10,7 @@ class ExceptionsController < ApplicationController # Give us an endpoint to use for 404 content in the ember app def not_found_body - render html: build_not_found_page(200, false) - end - - private - - def hide_search - @hide_search = true if SiteSetting.login_required + render html: build_not_found_page(status: 200) end end diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index c94520d3bd..312b721fb8 100644 --- a/app/controllers/list_controller.rb +++ b/app/controllers/list_controller.rb @@ -368,7 +368,13 @@ class ListController < ApplicationController raise Discourse::NotFound.new("category not found", check_permalinks: true) if !@category @description_meta = @category.description_text - raise Discourse::NotFound unless guardian.can_see?(@category) + if !guardian.can_see?(@category) + if SiteSetting.detailed_404 + raise Discourse::InvalidAccess + else + raise Discourse::NotFound + end + end if use_crawler_layout? @subcategories = @category.subcategories.select { |c| guardian.can_see?(c) } diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 981a3454ad..9de9c79592 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -79,12 +79,54 @@ class TopicsController < ApplicationController begin @topic_view = TopicView.new(params[:id] || params[:topic_id], current_user, opts) - rescue Discourse::NotFound + rescue Discourse::NotFound => ex if params[:id] topic = Topic.find_by(slug: params[:id].downcase) return redirect_to_correct_topic(topic, opts[:post_number]) if topic end - raise Discourse::NotFound + + raise ex + rescue Discourse::NotLoggedIn => ex + if !SiteSetting.detailed_404 + raise Discourse::NotFound + else + raise ex + end + rescue Discourse::InvalidAccess => ex + # If the user can't see the topic, clean up notifications for it. + Notification.remove_for(current_user.id, params[:topic_id]) if current_user + + deleted = guardian.can_see_topic?(ex.obj, false) || + (!guardian.can_see_topic?(ex.obj) && + ex.obj&.access_topic_via_group && + ex.obj.deleted_at) + + if SiteSetting.detailed_404 + if deleted + raise Discourse::NotFound.new( + 'deleted topic', + custom_message: 'deleted_topic', + status: 410, + check_permalinks: true, + original_path: ex.obj.relative_url + ) + elsif !guardian.can_see_topic?(ex.obj) && group = ex.obj&.access_topic_via_group + raise Discourse::InvalidAccess.new( + 'not in group', + ex.obj, + custom_message: 'not_in_group.title_topic', + group: group + ) + end + + raise ex + else + raise Discourse::NotFound.new( + nil, + check_permalinks: deleted, + original_path: ex.obj.relative_url + ) + end end page = params[:page] @@ -120,27 +162,6 @@ class TopicsController < ApplicationController end perform_show_response - - rescue Discourse::InvalidAccess => ex - if !guardian.can_see_topic?(ex.obj) && guardian.can_get_access_to_topic?(ex.obj) - return perform_hidden_topic_show_response(ex.obj) - end - - if current_user - # If the user can't see the topic, clean up notifications for it. - Notification.remove_for(current_user.id, params[:topic_id]) - end - - if ex.obj && Topic === ex.obj && guardian.can_see_topic_if_not_deleted?(ex.obj) - raise Discourse::NotFound.new( - "topic was deleted", - status: 410, - check_permalinks: true, - original_path: ex.obj.relative_url - ) - end - - raise ex end def publish @@ -960,19 +981,6 @@ class TopicsController < ApplicationController end end - def perform_hidden_topic_show_response(topic) - respond_to do |format| - format.html do - @topic_view = nil - render :show - end - - format.json do - render_serialized(topic, HiddenTopicViewSerializer, root: false) - end - end - end - def render_topic_changes(dest_topic) if dest_topic.present? render json: { success: true, url: dest_topic.relative_url } diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 8f552871c6..629faaa33c 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -606,8 +606,6 @@ class UsersController < ApplicationController end end end - rescue RateLimiter::LimitExceeded => e - render_rate_limit_error(e) rescue ::Webauthn::SecurityKeyError => err render json: { message: err.message, diff --git a/app/models/category.rb b/app/models/category.rb index 9efc7ab0f2..a7d2fe10a1 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -264,6 +264,15 @@ class Category < ActiveRecord::Base end end + def access_category_via_group + Group + .joins(:category_groups) + .where("category_groups.category_id = ?", self.id) + .where("groups.public_admission OR groups.allow_membership_requests") + .order(:allow_membership_requests) + .first + end + def duplicate_slug? Category.where(slug: self.slug, parent_category_id: parent_category_id).where.not(id: id).any? end diff --git a/app/serializers/hidden_topic_view_serializer.rb b/app/serializers/hidden_topic_view_serializer.rb deleted file mode 100644 index 414299f7e7..0000000000 --- a/app/serializers/hidden_topic_view_serializer.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -class HiddenTopicViewSerializer < ApplicationSerializer - attributes :view_hidden? - - has_one :group, serializer: BasicGroupSerializer, root: false, embed: :objects - - def view_hidden? - true - end - - def group - object.access_topic_via_group - end -end diff --git a/app/views/exceptions/not_found.html.erb b/app/views/exceptions/not_found.html.erb index a1173a8feb..805b1c96b9 100644 --- a/app/views/exceptions/not_found.html.erb +++ b/app/views/exceptions/not_found.html.erb @@ -1,4 +1,12 @@ -

<%= t 'page_not_found.title' %>

+
+

<%= @title %>

+ + <%- if @group&.allow_membership_requests %> + <%= SvgSprite.raw_svg('user-plus') %> <%= I18n.t('not_in_group.request_membership') %> + <%- elsif @group&.public_admission %> + <%= SvgSprite.raw_svg('user-plus') %> <%= I18n.t('not_in_group.join_group') %> + <%- end %> +
<%= build_plugin_html 'server:not-found-before-topics' %> diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 8ef26ec725..59f48d8d3a 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -252,6 +252,12 @@ en: invalid_grant_badge_reason_link: "External or invalid discourse link is not allowed in badge reason" email_template_cant_be_modified: "This email template can't be modified" invalid_whisper_access: "Either whispers are not enabled or you do not have access to create whisper posts" + not_in_group: + title_topic: "You must be in a group to see this topic." + title_category: "You must be in a group to see this category." + request_membership: "Request Membership" + join_group: "Join Group" + deleted_topic: "Oops! This topic has been deleted and is no longer available." reading_time: "Reading time" likes: "Likes" @@ -1412,6 +1418,7 @@ en: notification_email: "The from: email address used when sending all essential system emails. The domain specified here must have SPF, DKIM and reverse PTR records set correctly for email to arrive." email_custom_headers: "A pipe-delimited list of custom email headers" email_subject: "Customizable subject format for standard emails. See https://meta.discourse.org/t/customize-subject-format-for-standard-emails/20801" + detailed_404: "Provides more details to users about why they can’t access a particular topic. Note: This is less secure because users will know if a URL links to a valid topic." enforce_second_factor: "Forces users to enable second factor authentication. Select 'all' to enforce it to all users. Select 'staff' to enforce it to staff users only." force_https: "Force your site to use HTTPS only. WARNING: do NOT enable this until you verify HTTPS is fully set up and working absolutely everywhere! Did you check your CDN, all social logins, and any external logos / dependencies to make sure they are all HTTPS compatible, too?" same_site_cookies: "Use same site cookies, they eliminate all vectors Cross Site Request Forgery on supported browsers (Lax or Strict). Warning: Strict will only work on sites that force login and use SSO." diff --git a/config/site_settings.yml b/config/site_settings.yml index ba2fed74cd..e0887b8b6d 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1315,6 +1315,7 @@ trust: default: true security: + detailed_404: false enforce_second_factor: client: true type: enum diff --git a/lib/discourse.rb b/lib/discourse.rb index 53674172f0..b7b65762fb 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -71,13 +71,18 @@ module Discourse # When they don't have permission to do something class InvalidAccess < StandardError - attr_reader :obj, :custom_message, :opts + attr_reader :obj + attr_reader :opts + attr_reader :custom_message + attr_reader :group + def initialize(msg = nil, obj = nil, opts = nil) super(msg) @opts = opts || {} - @custom_message = opts[:custom_message] if @opts[:custom_message] @obj = obj + @custom_message = opts[:custom_message] if @opts[:custom_message] + @group = opts[:group] if @opts[:group] end end @@ -86,12 +91,15 @@ module Discourse attr_reader :status attr_reader :check_permalinks attr_reader :original_path + attr_reader :custom_message + + def initialize(msg = nil, status: 404, check_permalinks: false, original_path: nil, custom_message: nil) + super(msg) - def initialize(message = nil, status: 404, check_permalinks: false, original_path: nil) @status = status @check_permalinks = check_permalinks @original_path = original_path - super(message) + @custom_message = custom_message end end diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index 89f5fe6e54..80cc2a3cf3 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -155,10 +155,6 @@ module TopicGuardian can_see_category?(topic.category) end - def can_see_topic_if_not_deleted?(topic) - can_see_topic?(topic, false) - end - def can_get_access_to_topic?(topic) topic&.access_topic_via_group.present? && authenticated? end diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index fbac791531..c762fb070b 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -145,6 +145,12 @@ RSpec.describe ApplicationController do topic = create_post.topic Permalink.create!(url: topic.relative_url, topic_id: topic.id + 1) topic.trash! + + SiteSetting.detailed_404 = false + get topic.relative_url + expect(response.status).to eq(404) + + SiteSetting.detailed_404 = true get topic.relative_url expect(response.status).to eq(410) end @@ -204,7 +210,7 @@ RSpec.describe ApplicationController do response_body = response.body expect(response_body).to include(I18n.t('page_not_found.search_button')) - expect(response_body).to have_tag("input", with: { value: 'nopenope' }) + expect(response_body).to have_tag("input", with: { value: 'nope nope' }) end it 'should not include Google search if login_required is enabled' do diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 804952ef25..1420083154 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1229,12 +1229,13 @@ RSpec.describe TopicsController do describe 'when topic is not allowed' do it 'should return the right response' do + SiteSetting.detailed_404 = true sign_in(user) get "/t/#{private_topic.id}.json" expect(response.status).to eq(403) - expect(response.body).to eq(I18n.t('invalid_access')) + expect(response.body).to include(I18n.t('invalid_access')) end end @@ -1379,108 +1380,224 @@ RSpec.describe TopicsController do end end - context 'anonymous' do - expected = { - normal_topic: 200, - secure_topic: 403, - private_topic: 403, - deleted_topic: 410, - deleted_secure_topic: 403, - deleted_private_topic: 403, - nonexist: 404, - secure_accessible_topic: 403 - } - include_examples "various scenarios", expected - end - - context 'anonymous with login required' do + context 'without detailed error pages' do before do - SiteSetting.login_required = true - end - expected = { - normal_topic: 302, - secure_topic: 302, - private_topic: 302, - deleted_topic: 302, - deleted_secure_topic: 302, - deleted_private_topic: 302, - nonexist: 302, - secure_accessible_topic: 302 - } - include_examples "various scenarios", expected - end - - context 'normal user' do - before do - sign_in(user) + SiteSetting.detailed_404 = false end - expected = { - normal_topic: 200, - secure_topic: 403, - private_topic: 403, - deleted_topic: 410, - deleted_secure_topic: 403, - deleted_private_topic: 403, - nonexist: 404, - secure_accessible_topic: 200 - } - include_examples "various scenarios", expected - end - - context 'allowed user' do - before do - sign_in(allowed_user) + context 'anonymous' do + expected = { + normal_topic: 200, + secure_topic: 404, + private_topic: 404, + deleted_topic: 404, + deleted_secure_topic: 404, + deleted_private_topic: 404, + nonexist: 404, + secure_accessible_topic: 404 + } + include_examples "various scenarios", expected end - expected = { - normal_topic: 200, - secure_topic: 200, - private_topic: 200, - deleted_topic: 410, - deleted_secure_topic: 410, - deleted_private_topic: 410, - nonexist: 404, - secure_accessible_topic: 200 - } - include_examples "various scenarios", expected - end - - context 'moderator' do - before do - sign_in(moderator) + context 'anonymous with login required' do + before do + SiteSetting.login_required = true + end + expected = { + normal_topic: 302, + secure_topic: 302, + private_topic: 302, + deleted_topic: 302, + deleted_secure_topic: 302, + deleted_private_topic: 302, + nonexist: 302, + secure_accessible_topic: 302 + } + include_examples "various scenarios", expected end - expected = { - normal_topic: 200, - secure_topic: 403, - private_topic: 403, - deleted_topic: 200, - deleted_secure_topic: 403, - deleted_private_topic: 403, - nonexist: 404, - secure_accessible_topic: 200 - } - include_examples "various scenarios", expected - end + context 'normal user' do + before do + sign_in(user) + end - context 'admin' do - before do - sign_in(admin) + expected = { + normal_topic: 200, + secure_topic: 404, + private_topic: 404, + deleted_topic: 404, + deleted_secure_topic: 404, + deleted_private_topic: 404, + nonexist: 404, + secure_accessible_topic: 404 + } + include_examples "various scenarios", expected end - expected = { - normal_topic: 200, - secure_topic: 200, - private_topic: 200, - deleted_topic: 200, - deleted_secure_topic: 200, - deleted_private_topic: 200, - nonexist: 404, - secure_accessible_topic: 200 - } - include_examples "various scenarios", expected + context 'allowed user' do + before do + sign_in(allowed_user) + end + + expected = { + normal_topic: 200, + secure_topic: 200, + private_topic: 200, + deleted_topic: 404, + deleted_secure_topic: 404, + deleted_private_topic: 404, + nonexist: 404, + secure_accessible_topic: 404 + } + include_examples "various scenarios", expected + end + + context 'moderator' do + before do + sign_in(moderator) + end + + expected = { + normal_topic: 200, + secure_topic: 404, + private_topic: 404, + deleted_topic: 200, + deleted_secure_topic: 404, + deleted_private_topic: 404, + nonexist: 404, + secure_accessible_topic: 404 + } + include_examples "various scenarios", expected + end + + context 'admin' do + before do + sign_in(admin) + end + + expected = { + normal_topic: 200, + secure_topic: 200, + private_topic: 200, + deleted_topic: 200, + deleted_secure_topic: 200, + deleted_private_topic: 200, + nonexist: 404, + secure_accessible_topic: 200 + } + include_examples "various scenarios", expected + end end + + context 'with detailed error pages' do + before do + SiteSetting.detailed_404 = true + end + + context 'anonymous' do + expected = { + normal_topic: 200, + secure_topic: 403, + private_topic: 403, + deleted_topic: 410, + deleted_secure_topic: 403, + deleted_private_topic: 403, + nonexist: 404, + secure_accessible_topic: 403 + } + include_examples "various scenarios", expected + end + + context 'anonymous with login required' do + before do + SiteSetting.login_required = true + end + expected = { + normal_topic: 302, + secure_topic: 302, + private_topic: 302, + deleted_topic: 302, + deleted_secure_topic: 302, + deleted_private_topic: 302, + nonexist: 302, + secure_accessible_topic: 302 + } + include_examples "various scenarios", expected + end + + context 'normal user' do + before do + sign_in(user) + end + + expected = { + normal_topic: 200, + secure_topic: 403, + private_topic: 403, + deleted_topic: 410, + deleted_secure_topic: 403, + deleted_private_topic: 403, + nonexist: 404, + secure_accessible_topic: 403 + } + include_examples "various scenarios", expected + end + + context 'allowed user' do + before do + sign_in(allowed_user) + end + + expected = { + normal_topic: 200, + secure_topic: 200, + private_topic: 200, + deleted_topic: 410, + deleted_secure_topic: 410, + deleted_private_topic: 410, + nonexist: 404, + secure_accessible_topic: 403 + } + include_examples "various scenarios", expected + end + + context 'moderator' do + before do + sign_in(moderator) + end + + expected = { + normal_topic: 200, + secure_topic: 403, + private_topic: 403, + deleted_topic: 200, + deleted_secure_topic: 403, + deleted_private_topic: 403, + nonexist: 404, + secure_accessible_topic: 403 + } + include_examples "various scenarios", expected + end + + context 'admin' do + before do + sign_in(admin) + end + + expected = { + normal_topic: 200, + secure_topic: 200, + private_topic: 200, + deleted_topic: 200, + deleted_secure_topic: 200, + deleted_private_topic: 200, + nonexist: 404, + secure_accessible_topic: 200 + } + include_examples "various scenarios", expected + end + end + end it 'records a view' do @@ -1637,7 +1754,7 @@ RSpec.describe TopicsController do [:json, :html].each do |format| get "/t/#{topic.slug}/#{topic.id}.#{format}", params: { api_key: "bad" } - expect(response.code.to_i).to be(403) + expect(response.code.to_i).to eq(403) expect(response.body).to include(I18n.t("invalid_access")) end end diff --git a/test/javascripts/acceptance/topic-anonymous-test.js.es6 b/test/javascripts/acceptance/topic-anonymous-test.js.es6 index cd1bade7c7..26b3d6fb3c 100644 --- a/test/javascripts/acceptance/topic-anonymous-test.js.es6 +++ b/test/javascripts/acceptance/topic-anonymous-test.js.es6 @@ -19,10 +19,7 @@ QUnit.test("Enter without an id", async assert => { QUnit.test("Enter a 404 topic", async assert => { await visit("/t/not-found/404"); assert.ok(!exists("#topic"), "The topic was not rendered"); - assert.ok( - find(".not-found").text() === "not found", - "it renders the error message" - ); + assert.ok(exists(".topic-error"), "An error message is displayed"); }); QUnit.test("Enter without access", async assert => {