Skip to content
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

passport 迁移 #5

Open
JacksonTian opened this issue Mar 2, 2018 · 19 comments
Open

passport 迁移 #5

JacksonTian opened this issue Mar 2, 2018 · 19 comments
Assignees

Comments

@JacksonTian
Copy link
Member

No description provided.

@atian25
Copy link
Member

atian25 commented Mar 3, 2018

可以用 egg-passport-github 和 egg-passport-local

@thonatos
Copy link
Member

thonatos commented Mar 3, 2018

这里要用 egg-passport 来做么?

@atian25
Copy link
Member

atian25 commented Mar 3, 2018

@thonatos
Copy link
Member

thonatos commented Mar 3, 2018

好的,那我开一个 feature-passport 分支

@thonatos
Copy link
Member

thonatos commented Mar 3, 2018

登录注册这边就改为下面这样两个部分吧?

  • controller.sign

    • 显示登录页面
    • 注册
  • passport

    • local
    • github
    • other

@atian25
Copy link
Member

atian25 commented Mar 3, 2018

cnode 本来就只支持 local 和 github

@thonatos
Copy link
Member

thonatos commented Mar 5, 2018

疑问

  • egg-passport 登录时如何传递成功/失败消息有点疑问
    • 返回 null 可跳转登录页,但目前消息传递有问题,可选实现为直接写 session
    • 若抛出异常(401),是否考虑使用通用的错误页面
  • 登录成功的页面需要认证后的数据,是否考虑通过中间件将 session 中的信息添加到 ctx.locals,减少路由中重复书写?

参考

done(null, user, message) 
// 此处不抛出异常则缺少 message, egg-passport 实现为 done(null, user)

@fengmk2
Copy link
Member

fengmk2 commented Mar 5, 2018

handler 抛异常会通过 done(err) 返回 https://github.com/eggjs/egg-passport/blob/0afce5b0d5fbc107730e10dad6aff915c03cf079/lib/passport.js#L63

当然,你可以在 handler 里面实现 try catch 自行决定如何封装错误处理。

是否自定义 401 页面逻辑,完全由应用自身决定。

user 为 null 肯定就是登录失败了。

登录成功的页面需要认证后的数据,是否考虑通过中间件将 session 中的信息添加到 ctx.locals,减少路由中重复书写?

passport 不应该管数据保存到那里,这些都是后续应用去接管的操作。

@fengmk2
Copy link
Member

fengmk2 commented Mar 5, 2018

而且第三方登录失败,基本不会跳转回来,都在第三方网站卡住了。。。

@atian25
Copy link
Member

atian25 commented Mar 5, 2018

@fengmk2 passport-local 是本地

@thonatos
Copy link
Member

thonatos commented Mar 5, 2018

@fengmk2

上面说的是两部分:

  • passport 通过以后获取的信息,可以考虑通过一个 egg 中间节来添加,渲染的时候不用重复在ctx.render(template, locals) 中添加,类似 router.get(path, middleware, controller.method)

  • passport-local 是 passport 的 handler 部分,这里如果不通过 done 方法传递 message,可以考虑直接写 message 到 session,是 passport 的 handler 处理了数据,和外部处理差不多,根据 @fengmk2 的建议,可以考虑通过封装 user 对象返回用户信息以及消息(验证成功/失败)并存储在 ctx.session.passport 中,此后可使用中间件可读取数据并消除 connect-flash 中使用ctx.session.message 带来的冲突

@atian25
Copy link
Member

atian25 commented Mar 6, 2018

cc @OnedayLiu

@JacksonTian
Copy link
Member Author

@atian25 你们考虑自己实现一个 passport 吗,我看了下底下依赖的,写得比较戳。

@atian25
Copy link
Member

atian25 commented Mar 20, 2018

@_@ 要问问 @fengmk2 了,是他 wrap 的 passportjs ,后者好像是 express-base 的, wrap 起来就...

@bestlong
Copy link

@fengmk2 使用 egg-passport-local 插件,試過直接拋異常如下:

// app.js
app.passport.verify(async (ctx, user) => {
  throw new Error('找不到 user');
}

在 npm run dev 執行的環境下,網頁就回應 500 狀態碼。

@OnedayLiu
Copy link

@bestlong 嗯,是个问题,在跟苏千大大讨论方案

@bestlong
Copy link

bestlong commented Apr 2, 2018

@OnedayLiu 我正好在研究當 verify 失敗時要如何處理錯誤訊息讓前端可以顯示

https://github.com/eggjs/egg-passport/blob/0afce5b0d5fbc107730e10dad6aff915c03cf079/lib/passport.js#L63-L76

  doVerify(req, user, done) {
    const hooks = this._verifyHooks;
    if (hooks.length === 0) return done(null, user);
    (async () => {
      const ctx = req.ctx;
      for (const handler of hooks) {
        user = await handler(ctx, user);
        if (!user) {
          break;
        }
      }
      done(null, user);
    })().catch(done);
  }

可以先考慮在上列的 doVerify 範圍內增加捕捉 exception 然後改跑 done(err, user)
這樣就可以用 throw new Error() 丟錯誤訊息了。

可惜目前 doVerify 的程式碼段落還不是我完全看得懂的語法,所以無法直接動手改程式送 PR 了。

@OnedayLiu
Copy link

done(null, user);

有考虑过在这增加捕捉错误信息,但是如果这里出错的话是直接被 passport.js 直接捕获的

@bestlong
Copy link

bestlong commented Apr 3, 2018

我試過改成這樣但還是不行,還是 500 錯誤跑到 node_modules/koa-onerror/index.js 出現 non-error thrown 錯誤訊息,看來我的認知還不夠完整

  doVerify(req, user, done) {
    const hooks = this._verifyHooks;
    if (hooks.length === 0) return done(null, user);
    (async () => {
      const ctx = req.ctx;
        for (const handler of hooks) {
          user = await handler(ctx, user);
          if (!user) {
            break;
          }
        }
        done(null, user);
      }
    })().catch(err => {
      done(err.message, user);
    });
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants