New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Beta] - Add Redirect Middleware #19

Merged
merged 4 commits into from Feb 12, 2018

Conversation

Projects
None yet
3 participants
@0xTim
Copy link
Member

0xTim commented Jan 17, 2018

Basic Redirect Middleware for sessions - redirects users to the supplied path if they are not authenticated.

@0xTim 0xTim requested a review from tanner0101 Jan 17, 2018

@0xTim 0xTim changed the title Add Redirect Middleware [Beta] - Add Redirect Middleware Jan 17, 2018

@tanner0101 tanner0101 added this to the 2.0.0 milestone Jan 23, 2018

@tanner0101 tanner0101 self-assigned this Jan 23, 2018

@@ -0,0 +1,17 @@
import Vapor

public struct RedirectMiddleware<A>: Middleware where A: Authenticatable {

This comment has been minimized.

@tanner0101

tanner0101 Jan 23, 2018

Member

Class and methods should have doc comments (///)


let path: String

public init(A authenticableType: A.Type = A.self, path: String) {

This comment has been minimized.

@tanner0101

tanner0101 Jan 23, 2018

Member

Static extension on Authenticatable to match the other middleware would be nice.

router.grouped(User.guard(redirectingTo: "/login")).get(...) { ... }
@Joannis

This comment has been minimized.

Copy link
Member

Joannis commented Jan 23, 2018

@0xTim the tests aren't passing (anymore).

@0xTim

This comment has been minimized.

Copy link
Member Author

0xTim commented Jan 23, 2018

@Joannis yeah I'll rebase my changes back on and get the tests working and comments addressed

@0xTim 0xTim force-pushed the auth3-redirect-middleware branch from 29049fb to 342f559 Jan 23, 2018

@0xTim

This comment has been minimized.

Copy link
Member Author

0xTim commented Jan 23, 2018

@Joannis @tanner0101 should be updated and tests passing again

@Joannis

This comment has been minimized.

Copy link
Member

Joannis commented Jan 28, 2018

Just a small set of my notes from a slack discussion:

  • RedirectMiddleware could be ambiguous with a Vapor/vapor middleware
  • RedirectMiddleware's main purpose isn't redirection but checking for authentication status
  • The middleware is protecting a route against unauthorised users
  • I think it could be expanded to support Authorization in the future, too
  • The login static method could be better off without a default parameter. People have their own routes.

I'd love an overload to the static func login with a Route parameter like so:

let loginRoute = router.get("login") { request in
  return request.view().render("login.leaf")
}

let middleware = RedirectMiddleware<CustomUser>.login(at: loginRoute)
let protected = router.grouped(middleware)

@tanner0101 , thoughts?

/// The path to redirect to
let path: String

/**

This comment has been minimized.

@tanner0101

tanner0101 Feb 9, 2018

Member

/// style, please 😁

@0xTim 0xTim force-pushed the auth3-redirect-middleware branch from 342f559 to 4c5eb9b Feb 10, 2018

@tanner0101 tanner0101 merged commit 2fbde19 into beta Feb 12, 2018

1 check failed

ci/circleci: linux Your tests failed on CircleCI
Details

@tanner0101 tanner0101 deleted the auth3-redirect-middleware branch Feb 12, 2018

@tanner0101 tanner0101 added this to Done in Vapor 3 Feb 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment