-
-
Notifications
You must be signed in to change notification settings - Fork 5
Force all developer application redirect URIs to use HTTPS #145
Conversation
Closes #146 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the approach LGTM - just run the linter & fix some weird formatting
@@ -1,20 +1,18 @@ | |||
class Developers::ApplicationsController < ApplicationController | |||
nested_layouts "layouts/admin" | |||
nested_layouts 'layouts/admin' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will fail linting
|
||
before_action do | ||
@developers = true | ||
end | ||
|
||
before_action except: [:index, :request, :provision, :create] do | ||
before_action except: %i[index request provision create] do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the rationale behind this change, feels less descriptive imo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uhhhh the weird formatter did that
if (current_application.provisional? || current_application.owner_id != current_user.id) && !current_user.admin? | ||
render plain: "403 Forbidden or Provisional Domain", status: 403 | ||
render plain: '403 Forbidden or Provisional Domain', status: 403 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing - will also fail linting
if !current_user.admin? | ||
render plain: "403 Forbidden", status: 403 | ||
end | ||
render plain: '403 Forbidden', status: 403 unless current_user.admin? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again
@@ -44,10 +42,16 @@ def destroy_scope | |||
scopes = @application.scopes.to_a | |||
scopes.delete(params[:scope]) | |||
@application.update!(scopes: Doorkeeper::OAuth::Scopes.from_array(scopes)) | |||
redirect_back(fallback_location: developers_applications_path(id: params[:id]), notice: "Destroyed scope #{params[:scope]}") | |||
redirect_back(fallback_location: developers_applications_path(id: params[:id]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change
@@ -83,14 +87,22 @@ def destroy | |||
end | |||
|
|||
def create | |||
@application = Doorkeeper::Application.new(name: params[:name], redirect_uri: params[:redirect_uri], confidential: true) | |||
@application = Doorkeeper::Application.new(name: params[:name], redirect_uri: params[:redirect_uri], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change
end | ||
|
||
@application = Doorkeeper::Application.new(name: params[:name], redirect_uri: params[:redirect_uri], | ||
plan: params[:plan], confidential: true, provisional: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird line break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the flashes are in layouts - not in individual views, so update it there
@@ -1,54 +1,40 @@ | |||
<h1 class="font-heading text-3 lg:text-4 xl:text-5">Request an application</h1> | |||
<br><br> | |||
|
|||
<% flash.each do |type, msg| %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing - flash in layout
form { max-width: 50vw; } .domain { font-size: 2rem; } label { font-size: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not minify css
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requesting changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Should also fix this sentry issue