Skip to content

Conversation

nagem
Copy link
Contributor

@nagem nagem commented Sep 11, 2017

Adds CAS Auth type support

Breaking Changes

None

Review Checklist

  • Tests were added to cover all code changes
  • Documentation was added / updated
  • Code and tests follow standards in CONTRIBUTING.md

@codecov-io
Copy link

codecov-io commented Sep 11, 2017

Codecov Report

Merging #927 into master will decrease coverage by 0.06%.
The diff coverage is 79.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #927      +/-   ##
==========================================
- Coverage    90.2%   90.13%   -0.07%     
==========================================
  Files          48       48              
  Lines        6419     6458      +39     
==========================================
+ Hits         5790     5821      +31     
- Misses        629      637       +8
Flag Coverage Δ
#python 90.13% <79.48%> (-0.07%) ⬇️
Impacted Files Coverage Δ
api/config.py 93% <ø> (ø) ⬆️
api/auth/authproviders.py 98.42% <100%> (+0.27%) ⬆️
api/web/base.py 87.34% <27.27%> (-2.93%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc24d12...28541a4. Read the comment docs.

Copy link
Contributor

@kofalt kofalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good; few small changes. I have not tested this myself 🙂

}

def validate_user(self, token):
config.log.warning('the config is {}\n\n'.format(self.config))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove when you're ready to merge, or add more text to specify

tree = ElementTree.fromstring(response)

# check to see if xml response labeled request as success
if tree[0].tag.endswith('authenticationSuccess'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a link to where this logic is sourced from - just a github link to cas library source is fine.

raise APIAuthProviderException('Auth provider ticket verification unsuccessful.')

if not username:
raise APIAuthProviderException('Auth provider did not provide username')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will these two exceptions be client-visible? Maybe remove "auth provider" from verbiage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use that generically throughout AuthProviders but it is not shown to the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this line has been removed.

@nagem nagem force-pushed the cas-auth branch 2 times, most recently from 4fbc4df to f0d7db9 Compare September 11, 2017 23:09
Copy link
Contributor

@kofalt kofalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments addressed

@ryansanford
Copy link
Contributor

@nagem Environmental var SCITRAN_SITE_INACTIVITY_TIMEOUT is now working. Could we change the name to make it obvious the value represents seconds, or at a minimum document that in sample.config? Thanks.

self.abort(401, 'Inactivity timeout')

# set last_seen to now
config.db.authtokens.update_one({'_id': cached_token['_id']}, {'$set': {'last_seen': timestamp}})
Copy link
Contributor

@kofalt kofalt Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have assumed that the user's last_seen is always getting updated on every request. Are you sure it needs to be set here? If so, was it not being updated elsewhere?

Copy link
Contributor

@ryansanford ryansanford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ryansanford ryansanford removed the request for review from ambrussimon September 29, 2017 15:38
@ryansanford ryansanford merged commit 3a9ade2 into master Sep 29, 2017
@ryansanford ryansanford deleted the cas-auth branch September 29, 2017 19:15
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