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

Set expires at for password credentials flow #210

Merged
merged 2 commits into from
Aug 21, 2017

Conversation

beornf
Copy link
Contributor

@beornf beornf commented Aug 20, 2017

Noticed that expires at is not set in the session by default unlike the authorize code, client credential and refresh flows.

@coveralls
Copy link

coveralls commented Aug 20, 2017

Coverage Status

Coverage increased (+0.02%) to 80.511% when pulling 49a58f6 on beornf:password-expiry into fa50c80 on ory:master.

@aeneasr
Copy link
Member

aeneasr commented Aug 21, 2017

Thank you! Would you mind adding a (failing) test case for this too?

@coveralls
Copy link

coveralls commented Aug 21, 2017

Coverage Status

Coverage increased (+0.05%) to 80.542% when pulling bcf912a on beornf:password-expiry into fa50c80 on ory:master.

@beornf
Copy link
Contributor Author

beornf commented Aug 21, 2017

Added a failing check to the final test case.

@aeneasr
Copy link
Member

aeneasr commented Aug 21, 2017

perfect, thank you!

@aeneasr aeneasr merged commit 461b38f into ory:master Aug 21, 2017
@kujenga
Copy link
Contributor

kujenga commented Nov 15, 2017

Hmm, I just upgraded our fosite dependency and this PR seems to have broken my app's use of fosite. The expiration time set in the session, which seems like it should be preferential, is now overridden by this value.

Previously, the behavior used the expiration times in the session set here:

fosite/session.go

Lines 46 to 69 in dd9398e

// DefaultSession is a default implementation of the session interface.
type DefaultSession struct {
ExpiresAt map[TokenType]time.Time
Username string
Subject string
}
func (s *DefaultSession) SetExpiresAt(key TokenType, exp time.Time) {
if s.ExpiresAt == nil {
s.ExpiresAt = make(map[TokenType]time.Time)
}
s.ExpiresAt[key] = exp
}
func (s *DefaultSession) GetExpiresAt(key TokenType) time.Time {
if s.ExpiresAt == nil {
s.ExpiresAt = make(map[TokenType]time.Time)
}
if _, ok := s.ExpiresAt[key]; !ok {
return time.Time{}
}
return s.ExpiresAt[key]
}

I can work around this be setting the configuration variable to the access token time that we want, but that seems to defeat the purpose of the custom expiration times in the session.

@kujenga
Copy link
Contributor

kujenga commented Nov 15, 2017

Unless I was perhaps misusing the ExpiresAt map before? I was pre-filling values and expecting those to be respected/used.

@aeneasr
Copy link
Member

aeneasr commented Nov 15, 2017

This might be #211

@beornf
Copy link
Contributor Author

beornf commented Nov 15, 2017

In the case of pre-filling the session expiration time it might help to wrap all session expiration time setters with a nil check:
if request.GetSession().GetExpiresAt(fosite.AccessToken).IsZero() ...

budougumi0617 added a commit to budougumi0617/fosite that referenced this pull request May 10, 2019
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

Successfully merging this pull request may close these issues.

4 participants