-
Notifications
You must be signed in to change notification settings - Fork 53
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
Rework examples and refactor session handling #322
Conversation
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.
Here are a few notes I'm adding to explain ideas behind my additions.
@@ -93,9 +95,11 @@ async function start () { | |||
} | |||
}) | |||
|
|||
// Add valid and beforeSave hooks here to ensure session is valid #TODO |
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.
Related to #321
I'd like to see if we're not missing anything about session validation when we have an external validation service to trust.
@@ -0,0 +1,5 @@ | |||
{ | |||
"auth.login.required.missing": "用户名/密码未填写", |
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.
I’ve mistakenly forgotten to make nested objects.
That'll do for now, I wanted to make error messages i18n without importing ALL translation from client-side.
/** | ||
* Handle possibility where token endpoint, at exp returns seconds instead of µ seconds | ||
*/ | ||
export const handleTokenExp = exp => { |
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 isn’t used here, but has been a solution when I lost a few hours in a day because my exp
int value was seconds, instead of milliseconds. And it was failing silently.
Such a handler could be useful to ensure discrepancy between backend implementation and expectation.
...restOfRequestConfig | ||
} = requestConfig | ||
let requestConfigObj = { | ||
timeout: consts.AXIOS_DEFAULT_TIMEOUT, |
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.
Another pain point was that timeout was too short, and wasn’t the same in both client/server.
Backends doesn’t always respond fast. Most notably ones in production with loads of data and across various networks.
const hasSession = maybeReq !== null && !!maybeReq.session | ||
let maybeAuthenticated = await store.getters.authenticated | ||
if (hasSession === true && maybeAuthenticated === false) { | ||
const { data } = await $axios.get('/hpi/auth/whois') |
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.
Related to #321 ; Delegate to Koa all data management.
}) | ||
.catch(error => { | ||
const errorMessage = error.toString() | ||
this.captchaSvg = `<small style="line-height:1em;display:block;">${errorMessage}</small>` |
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.
There was no error management, now that makes it be more elegant if it breaks.
logout () { | ||
this.$store.dispatch('logout') | ||
} | ||
refreshCaptcha = debounce(this.getCaptcha, 500) // Note to you, reader, does this still work? |
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.
Does this really work?
@@ -1,26 +0,0 @@ | |||
import axios from 'axios' |
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.
Not needed anymore, due to proposed design change.
try { | ||
const { data: { access_token: token } } = await axios.post('/hpi/auth/login', { | ||
await axios.post('/hpi/auth/login', { |
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.
Post ouf things to Koa.
If successful, it'll create a cookie payload that's a JSON string that has the JWT token in.
Since it's going to be HttpOnly, AND a Signed Cookie, we're sure the token will transit only via the cookie, and nowhere else (ideally).
if (fakeIsValid) { | ||
validated.UserInfo = { | ||
UserName: 'admin', | ||
DisplayName: 'Haaw D. Minh', |
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 is a joke I’ve created to myself and left there.
If this is inappropriate, please tell me :)
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.
Talking about easter egg in my contributions.
In my previous PR, I've added a "city" called Lyster. I used an utf based emojii of a palm tree for Chinese.
The joke being that Lyster is a small town here in Québec (where my family is from), and we call it "Lyster Beach". It is in the middle of the land, it's only a church, and farms. And I wanted to have only one symbol to illustrate a city name, and thought it could be a funny thing to do.
We can remove those if you think it's inappropriate.
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.
Thank you so much for the contributions, will start to read them asap
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
Any recent attempt at building a production build from master? |
@renoirb |
I could build on master (where you merged my previous PR). Issues was about About Could you run my PR code with it? I may need help there. |
Hi! Sorry for delay, just a quick note that I haven't forgotten reviewing. |
Expanded examples in place and fixed things broken in my previous patch-set. I noticed that some examples were not all behaving without errors in the logs, although I'm 100% sure what is unfinished and what isn’t.
This PR fix found errors, adds things, and improves structure. (Hopefully)
Summary of the changes
Left lots of notes to capture how things are meant, to help future users
Minor things
Added
examples/activity/NewActivity.vue
now saves change to Vuex (but nothing reads it, yet)account/token.vue
to visualize session cookie set by Koa (ctx.session.jwt
)Major change
Handling session state
Removed client side authentication, Koa handles authentication and exposes what is possible.
See
#JwtTokenAreTooValuableToBeNonHttpOnly
in the code, it is inspired by the idea of NOT using localStorage/sessionStorage at all.Besides the new
/hpi/auth/token
(that should have validation to respond than), I feel this would be an improvement in ways to teach how to handle session state. — I'm open to discussing about it, maybe this design isn’t good.Sharing account information from external source
Notice the
keysMapWithLodashPathAndDefault
key, it is a way where one could use either data from JWT token payload, OR from a validation service available to Nuxt/Koa from a private network.