No user isn't an admin user anymore #991

Merged
merged 3 commits into from Apr 15, 2016

Conversation

Projects
None yet
3 participants
@Calvinp
Contributor

Calvinp commented Apr 7, 2016

The admin check would give you admin rights if you were logged in as no user.
This fixes that.

@tpetr please take a look - it is possible that there is a good reason for what Singularity was doing that I don't know about.

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Apr 7, 2016

Member

👍 I can't think of a reason where having no user should allow you any admin actions

Member

ssalinas commented Apr 7, 2016

👍 I can't think of a reason where having no user should allow you any admin actions

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Apr 7, 2016

Member

LGTM, thanks. Let's be sure to test out the endpoints that use this method in the test cluster to make sure there aren't any side effects.

Member

tpetr commented Apr 7, 2016

LGTM, thanks. Let's be sure to test out the endpoints that use this method in the test cluster to make sure there aren't any side effects.

@Calvinp Calvinp added the hs_staging label Apr 7, 2016

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Apr 7, 2016

Member

On second thought, Unauthorized is the more correct response to return if the user is not present: http://stackoverflow.com/questions/3297048/403-forbidden-vs-401-unauthorized-http-responses

I'd suggest copying what we do elsewhere and use the checkUnauthorized() method before checkForbidden()

Member

tpetr commented Apr 7, 2016

On second thought, Unauthorized is the more correct response to return if the user is not present: http://stackoverflow.com/questions/3297048/403-forbidden-vs-401-unauthorized-http-responses

I'd suggest copying what we do elsewhere and use the checkUnauthorized() method before checkForbidden()

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Apr 7, 2016

Member

we should be checking for unauthorized regardless of what adminGroups is

we should be checking for unauthorized regardless of what adminGroups is

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Apr 7, 2016

Member

🎈

Member

tpetr commented Apr 7, 2016

🎈

@Calvinp Calvinp added the hs_qa label Apr 8, 2016

@ssalinas ssalinas modified the milestone: 0.6.0 Apr 11, 2016

@Calvinp Calvinp added the hs_stable label Apr 14, 2016

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Apr 15, 2016

Member

Thanks for the fix @Calvinp

Member

ssalinas commented Apr 15, 2016

Thanks for the fix @Calvinp

@ssalinas ssalinas merged commit b495521 into master Apr 15, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ssalinas ssalinas deleted the no-user-isnt-admin branch Apr 15, 2016

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